compute_qname handle case where name could be unbound by tgbugs · Pull Request #2169 · RDFLib/rdflib (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

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

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.

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.

I will merge tomorrow if there is no further feedback.