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 issue 1808 #1821

Merged
merged 13 commits into from
Apr 18, 2022
Merged

Fix issue 1808 #1821

merged 13 commits into from
Apr 18, 2022

Conversation

edmondchuc
Copy link
Contributor

@edmondchuc edmondchuc commented Apr 15, 2022

Summary of changes

Fixes #1808

Essentially we are reverting back to returning a URIRef in the URIRef.skolemize() method and changing how Graph.de_skolemize() method converts subjects and objects of a statement to a BNode.

I think this change is much safer than changing what is returned by URIRef.skolemize() and potentially affecting other parts of the library. I've had a search through the codebase and nothing else touches the Graph.de_skolemize().

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Updated relevant documentation to avoid inaccuracies.
    • Considered adding additional documentation.
    • Considered adding an example in ./examples for new features.
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@aucampia
Copy link
Member

@edmondchuc - pre commit errors can be fixed by making a comment "pre-commit.ci autofix"

image

@nicholascar
Copy link
Member

pre-commit.ci autofix

@edmondchuc
Copy link
Contributor Author

Thanks for the tip @aucampia and thanks @nicholascar for commenting.

I'm hoping this PR fixes both #1808 and #1404 without introducing further side effects.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Nice work.

@aucampia
Copy link
Member

Will review in about an hour, just fixing some other issues.

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

pre-commit.ci autofix

rdflib/graph.py Outdated Show resolved Hide resolved
rdflib/graph.py Outdated Show resolved Hide resolved
@aucampia
Copy link
Member

@edmondchuc I have added some more tests now - and I'm fairly comfortable with the change after adding them - hope they are okay with you.

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.

Fix looks good, thanks @edmondchuc

@aucampia
Copy link
Member

Planning to merge on or before 2022-04-17 if there is no further feedback.

@edmondchuc
Copy link
Contributor Author

@edmondchuc I have added some more tests now - and I'm fairly comfortable with the change after adding them - hope they are okay with you.

Thanks for adding the additional tests @aucampia.

@aucampia
Copy link
Member

Won't merge before #1838

@aucampia aucampia merged commit 8a92d35 into RDFLib:master Apr 18, 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.

RDFLibGenid not matched by Graph.query()
3 participants