-
Notifications
You must be signed in to change notification settings - Fork 72
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@@ -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: |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy ran fine
There was a problem hiding this comment.
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!
Not sure what's up with codecov. It says rate limiting and the token size is 0. Is |
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. |
Could be because this repo is a fork, codecov/feedback#358 |
call Sequence.length/iter_parse fewer times
before:
after