Various cleanups #336
Various cleanups #336
Conversation
| logger.debug("data wrapper de-duplication: " | ||
| "%d encountered, %d kept, %d eliminated", | ||
| data_wrappers_encountered, | ||
| len(data_wrapper_cache), |
There was a problem hiding this comment.
Superficial: Violates PEP8. Also, there is https://github.com/Vimjas/vim-python-pep8-indent that I found to be helpful.
There was a problem hiding this comment.
It's no worse than before. :)
TBH: I do not understand the rationale for this PEP8 recommendation:
When using a hanging indent the following should be considered; there should be no arguments on the first line and further indentation should be used to clearly distinguish itself as a continuation line.
Using this code as an example, I don't find the arguments after the ( easy to miss, however I find the waste of vertical space quite noticeable (which I find at a premium even on my terminal, which has 104 rows available, cf. stty size). That's why I typically disable this rule in flake8. I also find this suggestion by flake8 to be puzzling:
foo = long_function_name(var_one, var_two,
var_three, var_four)because it wastes horizontal and vertical space. I can see where the parenthesis is just fine. :)
There was a problem hiding this comment.
It's no worse than before
That's true :-)
because it wastes horizontal and vertical space.
That's fair. I personally find it easier to read arguments aligned with the parentheses. If the horizontal space starts becoming a concern one could insert a line break before the first argument.
But, it's subjective and so resolving :).
There was a problem hiding this comment.
I'll proceed here, but I'd like to summon @alexfikl for an opinion. I don't need to ram through my opinion here.
There was a problem hiding this comment.
I'm mostly on the same page (don't particularly appreciate that recommendation) because
- even on moderately long function names it wastes space and if it's nested a bit it just gets worse and worse,
- doesn't survive renamings very well (especially with sed): two lines will need to be modified instead of one.
black doesn't align things either (from what I remember), but that's neither here nor there..
There was a problem hiding this comment.
Thanks for chiming in! I propose we stick with the current, "comfortably ambiguous" way, and leave the Flake8 rule disabled.
|
(I'll wait for your responses here before merging.) |
|
LGTM from my end, thanks! We could just do "Enable auto-merge[rebase]" which will avoid us having to update the branch, but if that's not your preferred path please merge as per your preference. |
If I do that, it just sits there. But there's a handy "update branch with rebase" button, which enables progress... so I clicked that. :) |
No description provided.