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

Isvaliduri optimization #1779

Merged
merged 3 commits into from Mar 30, 2022
Merged

Isvaliduri optimization #1779

merged 3 commits into from Mar 30, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 28, 2022

Summary of changes

Suggested minor optimization of namespace/__init__.py::compute_qname - only perform the check for uri validity if the uri is not aleady in self.__cache. Unsure how to test this specifically.

Checklist

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

Copy link
Member

@nicholascar nicholascar left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Passes all tests so I don't suppose it can be too wrong!

I think we need more documentation in namespace/__init__.py though. What is the purpose of __cache? Sure, it can be discovered by reading all the code but this is hard. This goes for trie also. I also don't see anything useful for cache or trie in the Sphinx docco either.

Perhaps @gjhiggins you can point to where some documentation is and I can have a go at adding it to the NamespaceManager class alongside all the docco I've just written for the new bind_namespaces parameter I'm adding in PR #1686.

@ghost
Copy link
Author

ghost commented Mar 30, 2022

I think we need more documentation in namespace/__init__.py though. What is the purpose of __cache? Sure, it can be discovered by reading all the code but this is hard. This goes for trie also. I also don't see anything useful for cache or trie in the Sphinx docco either.

AIUI, both cache and trie were introduced purely for internal efficency/speed and never intended to be exposed in the public API. Blame/credit tgbugs for the Mar 2020 implementation in commit #ee5ffb9

Perhaps @gjhiggins you can point to where some documentation is and I can have a go at adding it to the NamespaceManager class alongside all the docco I've just written for the new bind_namespaces parameter I'm adding in PR #1686.

I don't know of any documentation other than the extant Namespaces and Bindings docco - and in the case of NamespaceManager.bind() the docstring (noting that override defaults to True):

    def bind(self, prefix, namespace, override=True, replace=False):
        """bind a given namespace to the prefix

        if override, rebind, even if the given namespace is already
        bound to another prefix.

        if replace, replace any existing prefix with the new namespace
        """

(FWIW, I'm planning to contribute a complete overhaul and re-org of the docs to accompany 7.0, aucampia's recent adjustment enabling the building of PR docs will be enormously useful in that context.)

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, also this code is quite well tested, the actual validation is not but that is not what is being changed, so not that concerned there, and I will make a PR to add tests for it shortly.

@aucampia aucampia merged commit 74ae2f7 into RDFLib:master Mar 30, 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