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.

Sign up for GitHub

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

Jump to bottom

fix: ConjunctiveGraph.serialize to format as TriG by default (#1674) #2373

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: ConjunctiveGraph.serialize to format as TriG by default (#1674) #2373

wants to merge 7 commits into from

Conversation

WhiteGobo
Copy link
Contributor

Created method ConjunctiveGraph.serialize(..., format='trig', ...).

Because of typing problems I replaced some annotations in the overloaded methods of 'rdflib.Graph.serialize', so that in subclasses serialize knows, that it returns type(self) and not Graph. I have changed this, because in the docs its said, that serialize returns itself and nothing is stated, that a new graph is returned.

Graph.serialize annotations are still incompatible with ConjunctiveGraph.serialize. So added 'type: ignore [overriden]' to let mypy skip that method. I dont understand, what the errorlog of mypy wants to tell me.

Summary of changes

Changed types in graph.serialize
Added rdflib.graph.conjunctiveGraph.serialize as overriden method.
let mypy skip new method.

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@WhiteGobo
Copy link
Contributor Author

Link to original issue

@aucampia
Copy link
Member

aucampia commented May 19, 2023 •
edited
Loading

@WhiteGobo thanks for the PR, one potential problem is that this may be considered breaking the public API [ref], and if so, it should go into version 7 which also needs many other changes [ref].

If ConjunctiveGraph.serialize will always fail without this change, then this change does not break the API in the sense that the current API is already broken.

The best way to establish this is to add a test that would fail without this PR.

@aucampia
fix: supercharged ConjunctiveGraph.serialize to format as TriG by def…
414b060 
…ault (RDFLib#1674)

Created method ConjunctiveGraph.serialize(..., format='trig', ...).

Because of typing problems I replaced some annotations in the
overloaded methods of rdflib.Graph.serialize, so that in subclasses
serialize knows, that it returns type(self) and not Graph.

Because Graph.serialize annotations are still incompatible with
ConjunctiveGraph.serialize added type: ignore [overriden]
to let mypy skip that method.
@aucampia aucampia force-pushed the conjunctivegraph_serialize_trig1674 branch from fa3327c to 414b060 Compare May 19, 2023 10:44
@aucampia aucampia added bug Something isn't working needs discussion This issue needs further discussion to find an optimal way to resolve it. fix Fixes an issue and removed bug Something isn't working labels May 19, 2023
@WhiteGobo
Copy link
Contributor Author

Seems fine to me to include the pull request later.
And i have found out, why the annotation check with mypy failed so i have pushed a patch onto my branch.
Should i squash it with the rest of my commit?

As far as i have understood you, the test for ConjunctiveGraph.serialize you mentioned in your comment here and in the comment in the original issue has nothing to do directly with my PR, right? Except that with that test my PR could be directly included.

@aucampia
Copy link
Member

Should i squash it with the rest of my commit?

No need, will do on merge.

As far as i have understood you, the test for ConjunctiveGraph.serialize you mentioned in your comment here and in the comment in the original issue has nothing to do directly with my PR, right? Except that with that test my PR could be directly included.

Well yes, not directly related to this, but still quite important. I think there is another bug hiding somewhere with serializing ConjunctiveGraphs using turtle, but I will figure that out. And if the other two conditions hold we can merge as is, I will add tests for it sometime, hopefully this weekend, but best to keep it in a separate PR because we should test it anyway. And then as you said, once that is tested this can go in.

@coveralls
Copy link

coveralls commented May 19, 2023 •
edited
Loading

Coverage Status

coverage: 91.052% (+0.005%) from 91.047%
when pulling 9688e4e on WhiteGobo:conjunctivegraph_serialize_trig1674
into bb17072 on RDFLib:main.

@aucampia
Copy link
Member

I made this issue now:

@aucampia
Copy link
Member

PRs to V6 is closed until further notice. See this for more details:

@aucampia aucampia added the on hold Progress on this issue is blocked by something. label May 21, 2023
@aucampia
Copy link
Member

PRs to V6 is closed until further notice. See this for more details:

We will be open for PRs again once this is resolved:

@aucampia aucampia removed the on hold Progress on this issue is blocked by something. label Jun 2, 2023
@aucampia aucampia mentioned this pull request Jun 8, 2023
Closed
@aucampia
Copy link
Member

@WhiteGobo still working through some stuff to get to a point where we can merge this, but it will take some time.

@aucampia aucampia self-assigned this Jun 19, 2023
@WhiteGobo
Copy link
Contributor Author

@aucampia dont worry, take your time. Ive seen some of your efforts from the last weeks and that must have been a bunch of extra work.
And i thought, this will be pulled at the earliest, when the whole dataset/conjunctive graph complex has been worked out.

@aucampia aucampia added the feedback wanted Feedback from RDFLib users and contributors is wanted. label Aug 1, 2023
@nicholascar
Copy link
Member

@WhiteGobo can you review the conflict here and see if the PR can be readied for merging now?

@WhiteGobo
Copy link
Contributor Author

Ill get to it till end of the weekend.

@WhiteGobo
Copy link
Contributor Author

ok conflicts are resolved. Checked tests again for current version.

@ashleysommer
Copy link
Contributor

@nicholascar should this go into v7.1 release or v8?
I don't think it is necessarily a bad breaking change, though it might affect some users. It is mostly typing changes, the main function change is the fallback to use "trig" format to serialize ConjuntiveGraph and Dataset, when no format is given. That sounds like a logical change to me.

@nicholascar
Copy link
Member

I don't think this breaks the API so it's fine for 7.x and with a 7.1.0 release soon, that seems appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashleysommer ashleysommer Awaiting requested review from ashleysommer

@nicholascar nicholascar Awaiting requested review from nicholascar

Assignees

@aucampia aucampia

Labels
feedback wanted Feedback from RDFLib users and contributors is wanted. fix Fixes an issue needs discussion This issue needs further discussion to find an optimal way to resolve it.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@WhiteGobo @aucampia @coveralls @nicholascar @ashleysommer