-
Notifications
You must be signed in to change notification settings - Fork 571
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
Updating namespace.py to solve issue #801 #1044
Conversation
Added acceptance clause for "%" in Allowed name chars.
Test file to increase the scope of n amespaces.
Were you able to check if this also results in valid results for the other serialization formats? #801 (comment) |
Yes on making the changes described in my code I was able to get valid results other serialization formats also. |
Sir, |
test/test_issue801
Outdated
Issue 715 - path query chaining issue | ||
Some incorrect matches were found when using oneOrMore ('+') and | ||
zeroOrMore ('*') property paths and specifying neither the | ||
subject or the object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment looks out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darrengarvey I have updated the comment
test/test_issue801
Outdated
g.bind('', example) | ||
node = BNode() | ||
g.add((node, example['first%20name'], Literal('John'))) | ||
print(g.serialize(format="turtle").decode()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be checking the result of the serialisation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darrengarvey I have added the assert statement for checking the result
Deleting test_issue801
Updated test file to test issue 801
Added assertions for testing issue RDFLib#801
test/test_issue801.py
Outdated
@@ -13,7 +13,8 @@ def test_issue_801(self): | |||
g.bind('', example) | |||
node = BNode() | |||
g.add((node, example['first%20name'], Literal('John'))) | |||
print(g.serialize(format="turtle").decode()) | |||
self.assertEqual(g.serialize(format="turtle").decode().split("\n")[-3] == '[] :first%20name "John" .', True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this instead be:
self.assertEqual(g.serialize(format="turtle").decode().split("\n")[-3],
'[] :first%20name "John" .')
That way if the test fails you get to see what both sides of the equality comparison are. Plus keeping the line length shorter and making the string easier to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darrengarvey I have applied the requested changes
test/test_issue801.py
Outdated
@@ -13,7 +13,8 @@ def test_issue_801(self): | |||
g.bind('', example) | |||
node = BNode() | |||
g.add((node, example['first%20name'], Literal('John'))) | |||
print(g.serialize(format="turtle").decode()) | |||
self.assertEqual(g.serialize(format="turtle").decode().split("\n")[-3] == '[] :first%20name "John" .', True) | |||
print (g.serialize(format="turtle").decode()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make test run output noisy. I don't see other tests in rdflib printing output, so do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darrengarvey I have applied the requested changes
Removed print statement Reformatting assert statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
(@aayush17002 I'm not a developer, but thought I'd review anyway)
@aayush17002 please merge in |
@nicholascar I have merged into the master branch again as suggested by you |
Added acceptance clause for "%" in Allowed name chars.
Closes #801