Conversation
picnixz
left a comment
There was a problem hiding this comment.
That's an interesting and cleaner way to make it work but maybe we could add some tests as well, just in case?
|
I think tests with accessing / resetting / deleting the attributes (they are publicly named even though they are not exposed, just in case someone is subclassing them [again they shouldn't be part of the public API but well.. you never know; and it would be good to just check that the patterns are the expected ones. But yes, also a small test for the descriptor with a fake pattern and check that its implementation is correct (well it should be AFAIK). |
vstinner
left a comment
There was a problem hiding this comment.
I'm not excited by this change, IMO it goes too far :-(
This change is an optimization, you should provide a benchmark to prove that the change is worth it.
@vstinner sorry for not including them. Benchmarks show a consistent large decrease in import time, around 10ms on a standard release build in Windows. See below for detailed numbers: Using Old: The same command, on the default Old: With hyperfine, Python 3.13.2: A |
|
This PR is stale because it has been open for 30 days with no activity. |
|
Can this change be made simpler by using |
I don't think so as the class body of the textwrap class will be executed at import time, so |
| whitespace = r'[%s]' % re.escape(_whitespace) | ||
| nowhitespace = '[^' + whitespace[1:] | ||
| wordsep_re = re.compile(r''' | ||
| whitespace = fr'[{_whitespace}]' |
There was a problem hiding this comment.
Half of this PR is code refactoring which makes the PR harder to review. I would prefer to have a separated PR just for the refactoring.
| __all__ = ['TextWrapper', 'wrap', 'fill', 'dedent', 'indent', 'shorten'] | ||
|
|
||
|
|
||
| class _cached_regex: |
There was a problem hiding this comment.
Can you add a comment explaining the purpose of this class (lazy import)?

This uses self-overwriting descriptors to implement compiled class-level patterns. An unorthodox approach, but I think cleaner than using
is Nonechecks everywhere. If this looks reasonable, I'll add NEWS etc.A