Skip to content
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

call length less in _wrap_chunks #274

Closed
wants to merge 1 commit into from
Closed

Conversation

grayjk
Copy link

@grayjk grayjk commented Jun 25, 2024

call Sequence.length/iter_parse fewer times

before:

         3117 function calls in 0.005 seconds

   Ordered by: cumulative time
   List reduced from 39 to 20 due to restriction <20>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.005    0.005 terminal.py:1215(wrap)
        1    0.000    0.000    0.005    0.005 textwrap.py:342(wrap)
        1    0.000    0.000    0.005    0.005 sequences.py:155(_wrap_chunks)
      777    0.004    0.000    0.004    0.000 sequences.py:431(iter_parse)

after

         2301 function calls in 0.004 seconds

   Ordered by: cumulative time
   List reduced from 39 to 20 due to restriction <20>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1    0.000    0.000    0.004    0.004 terminal.py:1215(wrap)
        1    0.000    0.000    0.004    0.004 textwrap.py:342(wrap)
        1    0.000    0.000    0.004    0.004 sequences.py:155(_wrap_chunks)
      585    0.003    0.000    0.003    0.000 sequences.py:431(iter_parse)

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.12%. Comparing base (167c34e) to head (b718b68).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   95.31%   95.12%   -0.20%     
==========================================
  Files           9        9              
  Lines        1025     1025              
  Branches      216      216              
==========================================
- Hits          977      975       -2     
- Misses         44       46       +2     
  Partials        4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -189,7 +189,7 @@ def _wrap_chunks(self, chunks):
break
cur_line.append(chunks.pop())
cur_len += chunk_len
if chunks and Sequence(chunks[-1], term).length() > width:
if chunks and chunk_len > width:
Copy link
Owner

@jquast jquast Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this time chunk_len represents the length of the last chunk that was "popped" and not the last chunk in chunks, but who knows, maybe that's a good thing? just need to add more tests with #273 and if it works then that's fine

on closer look, this is fine, because of if chunks predicate that this only tests true on the break statement in the while loop, looks like a fine optimization, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting the linter to complain since chunk_len is defined within the while loop. I'm not sure if it's smart enough to know it's guarded or what, but it's not complaining.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the linter will catch it. mypy will complain about it - but that's because mypy doesn't understand the if chunks aspect

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy ran fine

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the dubious use of chunk_len outside of while loop, I've opted to incorporate the performance enhancement but move it to the inside of while loop in PR #275, thanks!

@avylove
Copy link
Collaborator

avylove commented Jun 26, 2024

Not sure what's up with codecov. It says rate limiting and the token size is 0. Is CODECOV_TOKEN still populated in secrets?

@jquast
Copy link
Owner

jquast commented Jun 26, 2024

Yes I saw that... when I re-executed an individual test it was fine, but when I re-executed all it hit the rate limit again. CODECOV_TOKEN is in secrets, its three years old.

@avylove
Copy link
Collaborator

avylove commented Jun 26, 2024

Could be because this repo is a fork, codecov/feedback#358
Not sure if this fix will make it to v3 or but supposedly they changed v4 to use the token even on forks. Only issue with v4 is it won't work with 2.7. We could add a workaround in the CI config to use v3 for 2.7 and v4 for everything else. I'm getting tired of all these CI workarounds, but 2.7 still had 27k downloads in the last month. That's about 1% of the total downloads.

@jquast
Copy link
Owner

jquast commented Jun 26, 2024

This change is incorporated and credited in #275, thanks @grayjk

@jquast jquast closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants