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 738: support empty publicID #1181

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

Conversation

AnjoMan
Copy link
Contributor

@AnjoMan AnjoMan commented Oct 7, 2020

This adds support for specifying a publicID as '';
instead of evaluating the falsy-ness of the input,
we want to use the provided string, even if it is
''.

Fixes #738

Proposed Changes

  • when the user specifies publicID as '', we should not trigger the None default behaviour

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 75.68% when pulling 7785f46 on AnjoMan:738-rdflib-ignores-empty-publicID into 56dc420 on RDFLib:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 75.68% when pulling 7785f46 on AnjoMan:738-rdflib-ignores-empty-publicID into 56dc420 on RDFLib:master.

@coveralls
Copy link

coveralls commented Oct 7, 2020

Coverage Status

Coverage decreased (-0.008%) to 75.659% when pulling 7785f46 on AnjoMan:738-rdflib-ignores-empty-publicID into 56dc420 on RDFLib:master.

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Oct 7, 2020

If you like this contribution, please add the #hacktoberfest-accepted label

@ashleysommer
Copy link
Contributor

Hi @AnjoMan
This looks like a good change, we will discuss this more formally tomorrow at our RDFLib maintainers meeting. We will have to run some more tests internally to ensure everything in the RDF/XML serializer operates correctly after this change.

I'm sorry, we are not participating in DigitalOcean's Hacktoberfest campaign.

@ashleysommer
Copy link
Contributor

Hi @AnjoMan
After discussing this, we have decided we will need to test this solution further. You have implemented the change just for the RDF/XML parser, but we need to test what happens if you use publicID="" for other parsers, like TRIG, or N3, or JSON-LD. While your change does fix the problem you are seeing, it could also potentially introduce breakages in unexpected places, so we need to be cautious about how to proceed with this.

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.

As can be seen for the turtle example, with your fix we would introduce some inconsistency across the serializers. The fix does also not work in the same way for the current Turtle parser/notation3 parser as we call graph.absolutize() in this case.

What is the usecase for an emptyID?

In m eyes this relative URI thing without explicitly defining a base in the file is a tough problem.

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.

This issue needs to be solved for all parsers at once.

@nicholascar nicholascar added the needs more work The PR needs more work before being merged or further reviewed. label Dec 7, 2021
This adds support for specifying a publicID as '';
instead of evaluating the falsy-ness of the input,
we want to use the provided string, even if it is
''.
@AnjoMan AnjoMan force-pushed the 738-rdflib-ignores-empty-publicID branch from 7a053b2 to 3f40a00 Compare November 24, 2023 21:53
@AnjoMan AnjoMan force-pushed the 738-rdflib-ignores-empty-publicID branch from fe59bfc to 8125658 Compare November 26, 2023 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more work The PR needs more work before being merged or further reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RDFLib doesn't accept an empty public ID
5 participants