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

Fix for #1824 s,http://rdlib.net,http://rdflib.net,g #1901

Merged
merged 11 commits into from May 19, 2022
Merged

Fix for #1824 s,http://rdlib.net,http://rdflib.net,g #1901

merged 11 commits into from May 19, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 9, 2022

Summary of changes

Fix for #1824 reported typo of http://rdlib.net/, change http://rdlib.net to http://rdflib.net in three files.

Checklist

  • Checked that there aren't other open pull requests for the same change.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@coveralls
Copy link

coveralls commented May 9, 2022

Coverage Status

Coverage increased (+0.0006%) to 88.423% when pulling 5247aae on gjhiggins:fix-for-issue1824 into ccb9c4a on RDFLib:master.

@ghost ghost self-requested a review May 9, 2022 21:56
@edmondchuc edmondchuc requested a review from a team May 10, 2022 00:25
@edmondchuc
Copy link
Contributor

Is this considered a breaking change?

@ghost
Copy link
Author

ghost commented May 10, 2022

Is this considered a breaking change?

Now that you mention it, probably yes, given that the skolemization oftimbl-card.n3pre- and post-change yields graphs that are not ==.

So, do I just close the PR and keep it around for when we're ready?

@edmondchuc
Copy link
Contributor

Not sure what to do. I just thought I'd mention it.

@niklasl
Copy link
Member

niklasl commented May 10, 2022

Given that neither domain is under the control of RDFLib, oughtn't we remake it further anyway? E.g. using rdflib.dev (if we bet on that being maintained during the lifetime of this project).

Not that skolemized blank ID:s "are to be" dereferenced, but as it is designed so (with a .well-known rather than e.g. a tag: URI), I believe the prudent and proper, qualitative way is to avoid "abanonable" domains...

@aucampia
Copy link
Member

Given that neither domain is under the control of RDFLib, oughtn't we remake it further anyway? E.g. using rdflib.dev (if we bet on that being maintained during the lifetime of this project).

I'm in favor of this, we should not use domains we don't own so rdflib.dev will be better another option may be rdflib.github.io.

@aucampia aucampia added the backwards incompatible will affect backwards compatibility, this includes things like breaking our interface label May 13, 2022
@ghost
Copy link
Author

ghost commented May 15, 2022

Given that neither domain is under the control of RDFLib, oughtn't we remake it further anyway? E.g. using rdflib.dev (if we bet on that being maintained during the lifetime of this project).

I'm in favor of this, we should not use domains we don't own so rdflib.dev will be better another option may be rdflib.github.io.

Changed.

Note to reviewers - although there were only a couple of corrected typos in the original PR, the wholesale change away from rdflib.net has resulted in the number of changed files increasing from 2 to 7.

@sonarcloud
Copy link

sonarcloud bot commented May 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aucampia
Copy link
Member

This PR includes the following changes with runtime impact:

  • Change the binding for genid that gets set under some circumstances:

    rdflib/rdflib/graph.py

    Lines 2051 to 2053 in c906f64

    self.bind(
    "genid", "http://rdflib.net" + rdflib_skolem_genid, override=False
    )

    I would consider this a breaking change as under some strange circumstances this could result in user code doing something different than before, but technically it was wrong before as it should match the prefixed used by BNode().skolemize() to have the desired effect, and it won't - so the current value is a bugged value to me anyway. I'm also not sure setting this in this place makes a lot of sense, I would say this should either always be set, or never. The current behavior for setting this seems somewhat arbitrary.

    My determination: Treat it as a non breaking change because the situations in which it would effect end user behavior is extremely arbitrary. In future we should re-consider what we are doing here though as I think it may be best to not do that, and either make genid part of our default namespace prefix set or not.

  • Change the user-agent comment used in some contexts:

    rdflib/rdflib/parser.py

    Lines 185 to 187 in c906f64

    headers = {
    "User-agent": "rdflib-%s (http://rdflib.net/; eikeon@eikeon.com)" % __version__
    }

    The user agent comment has no specific semantics associated with it in IETF RFC 7231 [ref] - however many systems do effectively assign some semantics to it. The comment in our user-agent however seems to not really follow the semantics that is assigned to it [ref].

    My determination: Treat it as a non breaking change as nothing mandates that semantics be assigned to the user-agent comment and in this case it is unclear why someone would assign semantics to it. Our user-agent comment anyway seems entirely arbitrary. In future I would recommend removing the user-agent comment entirely as it is unclear what benefit it serves.

  • Change the default skolemization authority:

    rdflib/rdflib/term.py

    Lines 484 to 485 in 8cf7494

    if authority is None:
    authority = "https://rdflib.github.io/"

    I don't see this as breaking, as there is nothing anywhere that I can find that assigns any semantic meaning to the authority used for generating Skolem IRIs. I also see our current value as a bug as we are not in control of the authority we currently use, which seems to go against what is said in the RDF 1.1 abstract syntax spec.

    My determination: Treat it as a non breaking change.

I would like to know if anyone differs on any of these points: @RDFLib/core

rdflib/graph.py Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented May 17, 2022

I would like to know if anyone differs on any of these points: @RDFLib/core

I'm in agreement.

This is to prevent a situation where different values are used for it in
different places.
@aucampia aucampia requested a review from edmondchuc May 17, 2022 15:06
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.

LGTM, as discussed earlier I don't think we should treat this as a breaking change, but open to more input on the matter.

This is to test the somewhat arbitrary behaviour of binding the `genid`
namespace when `Dataset.graph` is used without an identifier.
@aucampia
Copy link
Member

@gjhiggins I also added an explicit test for this behavior:

rdflib/rdflib/graph.py

Lines 2047 to 2056 in 7c51027

def graph(self, identifier=None, base=None):
if identifier is None:
from rdflib.term import _SKOLEM_DEFAULT_AUTHORITY, rdflib_skolem_genid
self.bind(
"genid",
_SKOLEM_DEFAULT_AUTHORITY + rdflib_skolem_genid,
override=False,
)
identifier = BNode().skolemize()

Test is here: https://github.com/gjhiggins/rdflib/blob/fix-for-issue1824/test/test_dataset/test_dataset.py#L239-L268

@aucampia aucampia removed the backwards incompatible will affect backwards compatibility, this includes things like breaking our interface label May 17, 2022
@aucampia aucampia requested a review from a team May 17, 2022 15:24
@aucampia aucampia added the review wanted This indicates that the PR is ready for review label May 17, 2022
@edmondchuc
Copy link
Contributor

I agree with your points @aucampia and I think using https://rdflib.github.io/ is a good idea.

Copy link
Member

@niklasl niklasl left a comment

Choose a reason for hiding this comment

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

Very sound reasoning; I also agree!

@aucampia aucampia merged commit 10f33ee into RDFLib:master May 19, 2022
@ghost ghost removed the review wanted This indicates that the PR is ready for review label Jun 4, 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.

Typo "http://rdlib.net/"
4 participants