RDFLib
fix: ConjunctiveGraph.serialize to format as TriG by default (#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. 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
- [x] Checked that there aren't other open pull requests for the same change.
- [x] 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).
- [x] Considered granting push permissions to the PR branch, so maintainers can fix minor issues and keep your PR up to date.
@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.
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.
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.
coverage: 91.052% (+0.005%) from 91.047% when pulling 9688e4ee9a30c11da6bcca7a6082e66b700f7dff on WhiteGobo:conjunctivegraph_serialize_trig1674 into bb170723b21c1cfb5b90f05b541d02be53867574 on RDFLib:main.
PRs to V6 is closed until further notice. See this for more details:
- https://github.com/RDFLib/rdflib/discussions/2395
PRs to V6 is closed until further notice. See this for more details:
We will be open for PRs again once this is resolved:
- https://github.com/RDFLib/rdflib/pull/2402
@WhiteGobo still working through some stuff to get to a point where we can merge this, but it will take some time.
@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.
@WhiteGobo can you review the conflict here and see if the PR can be readied for merging now?
@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.
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.