Conversation
package/MDAnalysis/core/AtomGroup.py
Outdated
There was a problem hiding this comment.
this line change is only done because of the trailing comment. Converting this into a normal comment in the above line will make yapf behave better.
There was a problem hiding this comment.
But that can be turned off.
|
I'm a bit confused. What issue is this PR addressing? Can you add a link and give a bit of explanation? |
|
https://groups.google.com/forum/#!topic/mdnalysis-devel/WjM0SN9gWFA It's to do with trying to solidify the style a bit more. It's not terrible, but in some places it's made it worse, and in others it's just replaced one flavour of ugly with another. I think with stuff like this you're always going to want to run it through a diff tool and accept/reject the proposed changes. |
|
Yes. Curiously in this file it even did a wrong transformation because the code wasn't PEP8 compliant Another approach which I have seen used in other old projects is to agree upon a style and then fix things as you go along. In all my current PR's I fix the warnings from flake8 for the functions I touch and leave the rest of the files alone. This takes longer until we would have a consistent style but we are also sure not to break things as we go along. I have my editor (emacs + elpy) set to highlight all lines containing syntax and formatting errors. Since the formatting errors are quite annoying to see this I have a personal motivation here to fix them. |
package/MDAnalysis/core/AtomGroup.py
Outdated
There was a problem hiding this comment.
Ah I noticed that yapf doesn't like these trailing comments. This is also the reason why some of the formatting comes out weird.
I changed the style a little bit to better represent what currently is in this file.
|
I tried changing the default style a little bit. It looks better to me but I still don't know why it splits the named assigns, tough I can live with it. |
|
I'm gonna kill this, I think we've kinda addressed this in #404 |
with default settings (PEP8 style).