Lazy-load syntax validators on attr access#8
Conversation
|
the |
|
I suppose I can move that too |
|
Moved. It is probably possible to go further and entirely avoid loading |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce rfc3987_syntax import time by deferring creation of Lark parsers/validators until they’re first accessed via module attribute access.
Changes:
- Removed eager creation of the shared
syntax_parserand term-specificis_valid_syntax_<term>helpers at import time. - Added module-level
__getattr__to lazily create and cache the shared parser and term-specific validator functions. - Updated
parse()to retrieve the lazily createdsyntax_parservia module attribute access.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __getattr__(name): | ||
| """ | ||
| Lazily create attributes, in particular syntax validators, when accessed. | ||
|
|
||
| is_valid_syntax_ucschar = make_syntax_validator("ucschar") | ||
| When an attribute like 'is_valid_syntax_iri' is accessed, this function |
There was a problem hiding this comment.
There are existing pytest tests for is_valid_syntax, but this PR adds a new public access pattern (lazy module attributes via __getattr__). Add coverage that imports and calls at least one term-specific helper (e.g. from rfc3987_syntax import is_valid_syntax_iri) and that accessing syntax_parser/a validator succeeds and returns a callable, so regressions in the lazy-loading mechanism are caught.
| if term_name not in RFC3987_SYNTAX_TERMS: | ||
| return None | ||
|
|
||
| is_valid_syntax_ipchar = make_syntax_validator("ipchar") | ||
| return make_syntax_validator(term_name) |
There was a problem hiding this comment.
get_syntax_validator only creates helpers when term_name is in RFC3987_SYNTAX_TERMS, but several grammar terms that previously had is_valid_syntax_<term> helpers (e.g. ipath_absolute, ipath_rootless, ipath_noscheme, ipath_empty, ipath) are not in that list. As a result, accessing those helpers will now raise AttributeError (breaking the documented API). Consider either expanding RFC3987_SYNTAX_TERMS to include all supported grammar rules that should have helpers, or removing this membership gate and letting make_syntax_validator/Lark determine validity (possibly with a clearer error for unknown terms).
| _attr_cache[name] = syntax_parser | ||
| return syntax_parser | ||
| if validator := get_syntax_validator(name): | ||
| _attr_cache[name] = validator |
There was a problem hiding this comment.
__getattr__ caches values only in _attr_cache but does not write them into the module namespace. This means every subsequent access to syntax_parser (and validators) will still go through __getattr__, adding avoidable overhead to hot paths like parse(). Consider also assigning the created object into globals()[name] (or setattr(sys.modules[__name__], name, value)) so future lookups bypass __getattr__.
| _attr_cache[name] = syntax_parser | |
| return syntax_parser | |
| if validator := get_syntax_validator(name): | |
| _attr_cache[name] = validator | |
| _attr_cache[name] = syntax_parser | |
| globals()[name] = syntax_parser | |
| return syntax_parser | |
| if validator := get_syntax_validator(name): | |
| _attr_cache[name] = validator | |
| globals()[name] = validator |
|
Hi! Since this repository is no longer actively maintained, I created and now maintain a fork of this package long-term:
Would you mind retargeting (or reopening) this pull request against the maintained fork so your fix/feature can be reviewed and merged there? Thank you! |
|
continued in jkowalleck#43 |
was solved via https://github.com/jkowalleck/rfc3987-syntax2/releases/tag/v1.3.0 |
|
this has been solved in maintained fork of the library since version 1.3.0 |
Currently this module has quite a large import time (over 0.5 sec on my machine).
This PR reduces this by lazy-loading the syntax validators lazily on attribute access.