-
Notifications
You must be signed in to change notification settings - Fork 132
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
Fix a KeyError in N-Quads normalization #52
base: master
Are you sure you want to change the base?
Conversation
@@ -1614,7 +1614,7 @@ def to_nquad(triple, graph_name=None): | |||
.replace('\"', '\\"')) | |||
quad += '"' + escaped + '"' | |||
if o['datatype'] == RDF_LANGSTRING: | |||
if o['language']: | |||
if o.get('language'): |
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.
How about if 'language' in o:
?
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.
I don't know what this check is actually for, but o['language']
is assuming the key exists and testing whether its value is falsy, which is why I changed it to o.get('language')
, which is also falsy (None) if the key doesn't exist.
'language' in o
would test only whether the key exists, which is orthogonal to what it was doing before. That doesn't necessarily mean it's wrong, though.
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.
From the discussion, it seems the PR should detect an incorrect form of the context. Currently, it will just ignore the issue.
There should be a test that runs this code path to avoid regressions. This was probably converted from js and there's never been a test run that didn't have the "language" key set. Do you have minimal JSON-LD and expected output that causes this issue? |
Aha! I've noticed that this is a bug in my context file -- I declared a value as a Minimal example: >>> example = {'@context': {'rdf': 'http://www.w3.org/1999/02/22-rdf-syntax-ns#', 'text': {'@id': 'http://example.com/text', '@type': 'rdf:langString'}}, 'text': 'foo'}
>>> jsonld.to_rdf(example, options={'format': 'application/nquads'})
Traceback (most recent call last):
File "<ipython-input-23-f64152bf1496>", line 1, in <module>
jsonld.to_rdf(example, options={'format': 'application/nquads'})
File "/home/rspeer/.virtualenvs/lum/lib/python3.5/site-packages/pyld/jsonld.py", line 295, in to_rdf
return JsonLdProcessor().to_rdf(input_, options)
File "/home/rspeer/.virtualenvs/lum/lib/python3.5/site-packages/pyld/jsonld.py", line 1147, in to_rdf
return self.to_nquads(dataset)
File "/home/rspeer/.virtualenvs/lum/lib/python3.5/site-packages/pyld/jsonld.py", line 1562, in to_nquads
quads.append(JsonLdProcessor.to_nquad(triple, graph_name))
File "/home/rspeer/.virtualenvs/lum/lib/python3.5/site-packages/pyld/jsonld.py", line 1617, in to_nquad
if o['language']:
KeyError: 'language' |
@@ -1614,7 +1614,7 @@ def to_nquad(triple, graph_name=None): | |||
.replace('\"', '\\"')) | |||
quad += '"' + escaped + '"' | |||
if o['datatype'] == RDF_LANGSTRING: | |||
if o['language']: | |||
if o.get('language'): |
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.
From the discussion, it seems the PR should detect an incorrect form of the context. Currently, it will just ignore the issue.
I wanted to try using this package with my new JSON-LD API, with URLs such as http://api.conceptnet.io/c/en/example .
It seems to be able to do simple transformations of the data, but it fails when I ask it to convert to N-Quads format:
Changing this to
o.get('language')
seems to fix the problem, so that's what this patch does.This patch passes the tests in
normalization/tests
. It doesn't passjson-ld.org/test-suite
, but it seems the master branch doesn't either.