Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Result.serialize work more like Graph.serialize #1418

Closed
wants to merge 1 commit into from

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Sep 21, 2021

DO NOT REVIEW: I'm going to split everything off from this that I can to reduce it to just the breaking changes so we can merge it before 7.0.0.

Reviewers beware: This patch is pretty big and contains a lot of things that could be merged separately. I may at some point break off some of the changes to separate PRs.

This patch makes the following changes to Result.serialize.

  • Return str by default instead of bytes.
  • Use "txt" as the default tabular serialization format.
  • Use "turtle" as the default graph serialization format.
  • Support both typing.IO[bytes] and typing.TextIO destinations.

Corresponding changes are made to the specific serializers also.

This patch also changes how text is written to typing.IO[bytes] in
serializers to ensure that the buffer is flushed and
detatched from the TextIOWrapper once the serialization function
completes so it can be used normally afterwards.

This patch further includes a bunch of additional type hints.

Fixes #1338

rdflib/graph.py Outdated Show resolved Hide resolved
test/test_serialize.py Outdated Show resolved Hide resolved
@aucampia aucampia force-pushed the iwana-20210912T2356 branch 7 times, most recently from eb17439 to 3875f78 Compare October 16, 2021 14:57
@aucampia aucampia changed the title Unify serialize() behaviours Make Result.serialize work more like Graph.serialize Oct 16, 2021
@aucampia aucampia force-pushed the iwana-20210912T2356 branch from 3875f78 to abb01be Compare October 16, 2021 15:15
@aucampia aucampia marked this pull request as ready for review October 16, 2021 15:21
@aucampia
Copy link
Member Author

@nicholascar as this breaks the interface I will try to break of a bunch of these changes and merge them independently so that this can be minimal in size and deferred to 7.x.

aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 16, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
@aucampia aucampia mentioned this pull request Oct 16, 2021
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 16, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
@nicholascar
Copy link
Member

@aucampia since you are doing work elsewhere, do you want to put this PR on hold or to close it perhaps?

aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 17, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
@aucampia
Copy link
Member Author

@nicholascar it's somewhat up to you. I feel this is ready to merge, if you only want to merge things that should go into 6.x then it is probably best to defer this, in which case I will proceed to break parts of this off into other PRs that won't break the interface and then proceed to have them merged. The other option is maybe to make a 6.x branch and then have master for 7.x but this may be more effort than it is worth.

If we keep master for 6.x until we are ready to start work on 7.x then I will mark this as draft and proceed with merging the parts of this that does not break the interface and rebase this appropriately so that it has a minimal diff.

@aucampia
Copy link
Member Author

I will anyway rebase this as other PRs are merged and keep this in a mergeable state at all times as best I can.

@nicholascar
Copy link
Member

@aucampia I see your other PRs with some of these parts in them, e.g. the type hinting additions. If we merge all those, I guess that will show the interface-breaking part here better. Then we can decide to merge into 6.0.x or 6.x.x or 7.x.

So let's just wait on this for all those type hinting ones to go through!

@aucampia aucampia marked this pull request as draft October 24, 2021 18:30
aucampia added a commit to aucampia/rdflib that referenced this pull request Oct 24, 2021
This commit only adds type hints and comments and does not make any changes that
should affect runtime.

The type hints added here derive from work done for RDFLib#1418.
@aucampia aucampia force-pushed the iwana-20210912T2356 branch from abb01be to 8729ed9 Compare January 2, 2022 00:09
@aucampia aucampia force-pushed the iwana-20210912T2356 branch 2 times, most recently from c4c7934 to 48322c8 Compare April 9, 2022 13:21
@aucampia
Copy link
Member Author

Closing PR, no need to keep it open, I'm busy splitting stuff off though but it just clogs up the view.

This patch makes the following changes to `Result.serialize`.

* Return str by default instead of bytes.
* Use "txt" as the default tabular serialization format.
* Use "turtle" as the default graph serialization format.
* Support both typing.IO[bytes] and typing.TextIO destinations.

Corresponding changes are made to the specific serializers also.

This patch also changes how text is written to typing.IO[bytes] in
serializers to ensure that the buffer is flushed and
detatched from the TextIOWrapper once the serialization function
completes so it can be used normally afterwards.

This patch further includes a bunch of additional type hints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make serialize() on a CONSTRUCT result act like normal g.serialize()
2 participants