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 passing ConjunctiveGraph as namespace_manager #2073

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

mielvds
Copy link
Contributor

@mielvds mielvds commented Aug 3, 2022

Summary of changes

  • passes the NamespaceManager of the ConjunctiveGraph as namespace_manager of the Graph created by the get_context() method instead of the ConjunctiveGraph object itself.

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 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 Aug 3, 2022

Coverage Status

Coverage remained the same at 90.447% when pulling 59ca17b on mielvds:patch-1 into cfa4180 on RDFLib:master.

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.

@mielvds thanks for the PR. While I think it is pretty safe and uncontroversial it would still be good to have tests for this.

Also, we are trying to reduce the effort it takes to make a release, and one part of this is to keep the changelog up to date always:

rdflib/CHANGELOG.md

Lines 201 to 217 in cfa4180

<!-- -->
<!-- -->
<!-- CHANGE BARRIER: START -->
<!-- -->
<!-- -->
- PLACEHOLDER.
Description of changes.
Closed [issue #....](https://github.com/RDFLib/rdflib/issues/).
[PR #....](https://ichard26.github.io/next-pr-number/?owner=RDFLib&name=rdflib).
<!-- -->
<!-- -->
<!-- CHANGE BARRIER: END -->
<!-- -->
<!-- -->

So it would be good if you could add tests and update the changelog, but I will try find some time this weekend to add tests and update the change on your PR branch if it is not done yet, as this is rather important to fix.

@aucampia
Copy link
Member

aucampia commented Aug 3, 2022

@mielvds please share how you managed to trigger this if you can.

@mielvds
Copy link
Contributor Author

mielvds commented Aug 4, 2022

Hi @aucampia
I agree. I will add some tests and update the changelog. You can then check if they suffice.

I triggered this while developing a documentation tool for shacl. I manage different shacl graphs as named graphs and got errors when printing the namespaces of individual graphs.

@mielvds mielvds requested a review from aucampia August 4, 2022 11:34
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.

Thank you very much for the test and changelog update @mielvds, the change looks good.

@aucampia aucampia requested a review from a team August 4, 2022 20:15
@aucampia aucampia merged commit cc80c9c into RDFLib:master Aug 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.

3 participants