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

Issue 1003 #1005

Merged
merged 8 commits into from Apr 17, 2020
Merged

Issue 1003 #1005

merged 8 commits into from Apr 17, 2020

Conversation

nicholascar
Copy link
Member

@nicholascar nicholascar commented Apr 16, 2020

Adds @base to N3 & Turtle serializations, if base is set in .serialize()

Closes #1003

@nicholascar nicholascar added the bug Something isn't working label Apr 16, 2020
@nicholascar nicholascar added this to the rdflib 6.0.0 milestone Apr 16, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 75.623% when pulling a8a0d71 on issue_1003 into acb9d10 on master.

@coveralls
Copy link

coveralls commented Apr 16, 2020

Coverage Status

Coverage increased (+0.008%) to 75.949% when pulling 308abf7 on issue_1003 into acb9d10 on master.

@nicholascar
Copy link
Member Author

close/open Travis

@nicholascar nicholascar reopened this Apr 16, 2020
Copy link
Member

@white-gecko white-gecko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very happy with these changes. For setting the base in the Graph class I would switch the overwriting behavior and for the Turtle serializer I don't like the side effect.

An argument for not changing the Turtle behaviors regarding setting self.base is that we should stick to the same behavior which we have for TriG (

def serialize(self, stream, base=None, encoding=None,
) and RDF/XML (
def serialize(self, stream, base=None, encoding=None, **args):
)

rdflib/graph.py Outdated Show resolved Hide resolved
rdflib/plugins/serializers/turtle.py Show resolved Hide resolved
@white-gecko
Copy link
Member

We should also make sure to have the base set for TriX, RDF/XML, and TriG (https://github.com/RDFLib/rdflib/tree/master/rdflib/plugins/serializers)

@nicholascar
Copy link
Member Author

We should also make sure to have the base set for TriX, RDF/XML, and TriG

OK, I'll make the same changes to all

…(). Added RDF/XML, TriX & 1/2 TriG (incomplete)
@nicholascar
Copy link
Member Author

I've made the changes I said I would above and got RDF/XML & TriX to work. TriG only 1/2 works since it can't express a base per graph, only one for the whole dataset/conjunctive graph. This means the full solution, which I have no time for, is to reverse QNAME any URI in any individual graph that doesn't use the same base as the dataset/conjunctive graph but the Turtle serializer, which the TriG serializer uses, doesn't keep track of things to make this easy!

So there is a loss of info if multiple graphs within a dataset/conjunctive graph declare base URIs different to the dataset/conjunctive graph and then serialize to TriG... future work!

@white-gecko
Copy link
Member

white-gecko commented Apr 16, 2020

So there is a loss of info if multiple graphs within a dataset/conjunctive graph declare base URIs different to the dataset/conjunctive graph and then serialize to TriG... future work!

I think this is not necessary. The base in general is not relevant info. It is just a convenience for serializations. So I think we do not need to go the extra mile here.

Setting the base on a Graph object is also not necessary but I think a nice feature to give a hint to the Serializers.

An I think the currently set base as attribute on a serialize method should always win over the base set in a graph. serialize(base=base) > graph.base > None.
But the graph forwards is base anyways:
https://github.com/RDFLib/rdflib/blob/master/rdflib/graph.py#L956-L979

@white-gecko
Copy link
Member

white-gecko commented Apr 17, 2020

@nicholascar I have made the changes in the way as I would expect the behavior. Is this ok for you? Maybe @ashleysommer should now review this pullrequest (we should anyhow merge it after the 5.0.0 release).

@nicholascar
Copy link
Member Author

Hi @white-gecko great, I was just about to make the changes you just made! I'll approve and merge as @ashleysommer's not available at the moment.

@nicholascar nicholascar merged commit 39d07c4 into master Apr 17, 2020
@nicholascar nicholascar deleted the issue_1003 branch April 17, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@base is not set
3 participants