| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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?
Sorry, something went wrong.
Happy to, do you mean for _cached_regex, or tests for TextWrapper, or? A |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
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.
Sorry, something went wrong.
@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 -X importtime -Sc 'import textwrap' Old: import time: 907 | 10796 | textwrap_current The same command, on the default PCbuild\build.bat build: Old: import time: 5272 | 29988 | textwrap_current With hyperfine, Python 3.13.2: PS> hyperfine -N --warmup 5 "python -c ''" "python -c 'import textwrap_current'" "python -c 'import textwrap_new'"
Benchmark 1: python -c ''
Time (mean ± σ): 24.1 ms ± 0.9 ms [User: 5.4 ms, System: 3.4 ms]
Range (min … max): 22.3 ms … 26.4 ms 124 runs
Benchmark 2: python -c 'import textwrap_current'
Time (mean ± σ): 34.6 ms ± 1.5 ms [User: 8.3 ms, System: 4.0 ms]
Range (min … max): 29.5 ms … 38.1 ms 85 runs
Benchmark 3: python -c 'import textwrap_new'
Time (mean ± σ): 24.2 ms ± 0.9 ms [User: 8.3 ms, System: 3.7 ms]
Range (min … max): 21.8 ms … 27.0 ms 124 runs
A |
Sorry, something went wrong.
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
Can this change be made simpler by using lazy import re? |
Sorry, something went wrong.
I don't think so as the class body of the textwrap class will be executed at import time, so re will be reified directly. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| __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)?
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This uses self-overwriting descriptors to implement compiled class-level patterns. An unorthodox approach, but I think cleaner than using is None checks everywhere. If this looks reasonable, I'll add NEWS etc.
A