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

N3 parsing of local names with special characters #523

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acassaignemondeca
Copy link

Hello,

the goal of this commit is to improve the handling of local names with special characters. I modified the implementation of the "qname" function in accordance to https://github.com/RDFLib/rdflib/blob/master/rdflib/plugins/parsers/notation3.py (function "qname").

Also to align with https://github.com/RDFLib/rdflib/blob/master/rdflib/plugins/parsers/notation3.py:

  • in _notQNameChars I removed . and %
  • I created _notKeywordsChars (= _notQNameChars + "." )
  • outside of qname function and to handle the side effect of changing _notQNameChars by _notKeywordsChars I replaced all occurrences of _notQNameChars by _notKeywordsChars.
  • I verified that "%" was not a concern in these cases. I could do that everywhere except for the "tok" function where the logic is not in notation3.py so I added back the "%" case. I don't know if this logic is useful ?

I tested the modification with rdflib-evaluation. No regression and tests succeeding are up from 193 to 211 (out of 298). (Tests attached to this pull request).

Best regards,
Arnaud

spec-rdflib-ntriples-patch.txt
spec-rdflib-ntriples-reference.txt
spec-rdflib-turtle-patch.txt
spec-rdflib-turtle-reference.txt

@angelo-v
Copy link
Contributor

Thanks for your contribution. I cannot say much about it contentwise and leave that up to @timbl .

From a quality perspective it is good to see more tests passing. I did not know rdflib-evaluation before. Could we include it in github actions? Nevertheless the code changes a lot and I would like to see this covered by unit tests as well. Those tests would also serve as a documentation of the changes so that it is easier to grasp how the code actually behaves.

@@ -168,7 +168,7 @@ $Id: n3parser.js 14561 2008-02-23 06:37:26Z kennyluck $

HAND EDITED FOR CONVERSION TO JAVASCRIPT

This module implements a Nptation3 parser, and the final
This module implements a Notation3 parser, and the final
part of a notation3 serializer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
part of a notation3 serializer.
part of a Notation3 serializer.

Copy link
Contributor

@angelo-v angelo-v left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I cannot say much about it contentwise and leave that up to @timbl .

From a quality perspective it is good to see more tests passing. I did not know rdflib-evaluation before. Could we include it in github actions? Nevertheless the code changes a lot and I would like to see this covered by unit tests as well. Those tests would also serve as a documentation of the changes so that it is easier to grasp how the code actually behaves.

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.

3 participants