-
Notifications
You must be signed in to change notification settings - Fork 565
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
Isvaliduri optimization #1779
Conversation
There was a problem hiding this 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.
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
I don't know of any documentation other than the extant Namespaces and Bindings docco - and in the case of 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.) |
There was a problem hiding this 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.
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 inself.__cache
. Unsure how to test this specifically.Checklist
so maintainers can fix minor issues and keep your PR up to date.