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

compute_qname handle case where name could be unbound #2169

Merged
merged 6 commits into from Dec 14, 2022

Conversation

tgbugs
Copy link
Contributor

@tgbugs tgbugs commented Nov 23, 2022

In NamespaceManager.compute_qname it is possible for the variable name to be unbound when a namespace is used as a predicate e.g. ns2: and the namespace itself cannot be split any further.

Fixing this usually just kicks the can down the road because the only reason to hit this condition is that you are trying to serialize a predicate that has no valid NCName form into an xml format that requires one, making it impossible to complete the conversion. That said, this fix should make it easier to understand what is going on by unmasking the real issue.

In NamespaceManager.compute_qname it is possible for the variable name
to be unbound when a namespace is used as a predicate e.g. ns2: and
the namespace itself cannot be split any further.

Fixing this usually just kicks the can down the road because the only
reason to hit this condition is that you are trying to serialize a
predicate that has no valid NCName form into an xml format that
requires one, making it impossible to complete the conversion.
That said, this fix should make it easier to understand what is going
on by unmasking the real issue.
@coveralls
Copy link

coveralls commented Nov 23, 2022

Coverage Status

Coverage increased (+0.0005%) to 90.633% when pulling 0d4f299 on tgbugs:fix-nm-name-error into f9d7132 on RDFLib:main.

@aucampia
Copy link
Member

aucampia commented Dec 4, 2022

pre-commit.ci autofix

tgbugs and others added 4 commits December 7, 2022 22:28
The new test graph will produce the UnboundLocalError with the old
code that is missing the name = '' assignment. The new code correctly
produces the xfail ValueError when trying to convert to xml.
more testing ...
Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

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

@tgbugs thanks for the fix, looks good.

And apologies for the dodgy commit names to your branch, I would fix them but I don't want to force push to your branch so I'm just leaving them as is. I will sqash merge this PR later.

@aucampia aucampia requested a review from a team December 13, 2022 23:29
@aucampia
Copy link
Member

I will merge tomorrow if there is no further feedback.

@aucampia aucampia merged commit db5c05a into RDFLib:main Dec 14, 2022
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.

None yet

3 participants