Correct behaviour of compute_qname for URNs by namcsi · Pull Request #1274 · 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

Conversation3 Commits2 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 }})

Fixes: Allow URN qnames to be computed correcly by compute_qname
Removed colon from ALLOWED_NAME_CHARS, as having it included results in URN identifiers never being split by split_uri and thus no qname being computed by compute_qname for URNs.
I've also added some tests for correct behaviour of compute_qname for URNs and turtle serialization (where this issue surfaced initally, see the end of #977)

This was referenced

Mar 8, 2021

Just a note: removing the colon from ALLOWED_NAME_CHARS doesn't break any existing tests, and passes the 2 new tests so it should be an easy fix:) The colon was added originally to modify the behavior of is_cname, and split_uri but was subsequently filtered out in the is_cname function (#663 (comment)) as it caused issues there too. Seems like it causes issues for split_uri as well, it just wasn't noticed due to no testing for URNs.

Coverage increased (+0.008%) to 75.496% when pulling 7cdd668 on nemesamade:master into 16b218b on RDFLib:master.

…and relatedly for correct turtle serialization of URNs

… identifiers not getting split at all and thus leading to compute_qname being unable to compute the qnames of URNs. This change did not break any current tests, and allowed the previously added 2 tests to pass (note: as a quick fix is_cname already excludes colon from ALLOWED_NAME_CHARS as it causes issues there too #663 , so as far as I can tell there really is no reason for colon to be included)

Choose a reason for hiding this comment

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

Great PR - thanks for diving in and fixing this.