-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor _collect_block_lines
#6560
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
Conversation
Pull Request Test Coverage Report for Build 2294120969
π - Coveralls |
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.
LGTM, nice new function.
"""Set the state of a message in a block of lines.""" | ||
first = node.fromlineno | ||
last = node.tolineno | ||
for lineno, state in list(lines.items()): |
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.
Is the call to list necessary ?
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.
That would be a nice lint warning π
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.
Ah, it necessary...
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.
It looked like an artifact from a 2to3 migration, my bad :)
Type of Changes
Description
Ref. #6556.
Main difference is that we now get the message definitions before calling/doing
_set_message_state_in_block
. It's a little cleaner and separates some functionality._collect_block_lines
becomes a bit pointless though.