Isvaliduri optimization#1779
Conversation
nicholascar
left a comment
There was a problem hiding this comment.
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.) |
aucampia
left a comment
There was a problem hiding this comment.
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.