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

Add namespace_manager argument for n3 method on Paths #2174

Merged
merged 3 commits into from Dec 14, 2022

Conversation

mgberg
Copy link
Contributor

@mgberg mgberg commented Dec 7, 2022

Summary of changes

The n3 method on the Term class optionally takes a NamespaceManager as an argument which, if provided, is used to create a prefixed name instead of fully qualified URIs for URIRefs and typed Literals where applicable if an appropriate prefix has been bound for the relevant namespace. Path objects also have a n3 method to create N3/SPARQL compatible serializations for those objects which call the n3 method on the underlying URIRefs. However, Path.n3 did not have the argument for the NamespaceManager.

This PR simply adds the same optional NamespaceManager argument to the n3 method for Paths to be consistent with the n3 method on Terms and passes that NamespaceManager to any nested calls to n3 so it behaves as expected.

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.

Sadly the __mul__ dunder is not defined in a way that mypy can find it,
so there are some type errors that arise from using it. I added
`type: ignore` comments for those.

I also added some more type hints, which while not strictly necessary,
does contribute to more complete type hint coverage.
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.

@mgberg the PR looks pretty good, I made some minor changes to get validation to pass and to add some additional type hints:

If you make PRs, it is better to make it from your personal account, that way we can push to your branch instead of needing to make PR to your branch.

chore: add some more type hints and ignores
@aucampia aucampia requested a review from a team December 13, 2022 18:28
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.

Change looks good to me, it makes sense to have the same signature for def n3() across all classes. I will merge before this weekend if there is no further feedback.

@aucampia aucampia requested a review from a team December 13, 2022 18:31
@mgberg
Copy link
Contributor Author

mgberg commented Dec 13, 2022

If you make PRs, it is better to make it from your personal account, that way we can push to your branch instead of needing to make PR to your branch.

I believe that my organizational policy disallows that, but I'll double check on that. Thanks!

@aucampia
Copy link
Member

I believe that my organizational policy disallows that, but I'll double check on that. Thanks!

It really is also up to you, I don't really mind either way, makes things a bit easier if we can push to your branch but it is not a requirement for PRs and if the PR does something valuable I don't mind making a PR to the PR branch to add fixes.

@aucampia aucampia merged commit def1c1f into RDFLib:main Dec 14, 2022
@mgberg mgberg deleted the add-nsm-arg-to-path-n3 branch January 25, 2023 20:58
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.

None yet

2 participants