Closed dreadatour closed 8 years ago
Hi there,
I understand where you're coming from, and appreciate you taking the time to submit this PR, but I don't think we should merge this in.
Style-only changes add a lot of noise, and I don't think the stylistic benefit offsets that noise. Furthermore, mixing semantic changes within a commit advertised as a style commit is a big no-no.
I'll let @koenbok pitch in and reopen if he has strong feelings otherwise.
As far as the semantic changes go, I'll have a look.
Generally speaking, I think optimization changes should be supported by evidence. For a list
, len
is a function call that basically does an attribute lookup; we don't typically cache all those in a local variable (unless there's a demonstrated performance benefit).
Cheers,
It is your choice. PEP8 allows people speak in one language. But no more words, anyone knows all benefits of style-guide. Thanks, anyway.
About len
and list
you misunderstood me. I think it is because of my bad english :)
Take a look at original file ;) You already did this optimisation, but it is missed somewhere - may be as a result of inaccurate merge, may be as a mistype.
At least you should remove this useless line if you don't want to optimise things: https://github.com/koenbok/Cactus/blob/master/cactus/utils/sync.py#L26
What semantic changes are you talking about?
@dreadatour,
Thanks for the correction about len
vs list
(I haven't worked on that specific part of the code, so I made the wrong assumption).
Regarding semantic changes, I'm referring to e.g. changing the name of a test so that it runs.
Thomas, thank you for explanation. The thing is I can't fix PEP8 violations without bring this semantic changes into code. I understand what you are talking about (mixing different types of changes), and agree with it.
Someone was supposed to do this.
Pros: almost all PEP8 violations are fixed (python developers would be happy :). Cons: future pull requests could be painful.
Note: I set max-line-length to 120 symbols (instead of 80) because where are many lines longer than 80 chars and it would be HUGE pull request if I will fix them all. Also not all developers like 80 chars limitation. It's up to you ;)