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
Blacken & Flake8 nml #103
Blacken & Flake8 nml #103
Conversation
646c809
to
d63415c
Compare
I did start reading the Black commit, obviously got bored, but I think it's great. Hopefully we can get this approved soon :) |
Can you give me a brief explanation of what problem this solves? |
Consistent formatting amongst NML itself, and OpenTTD python as a whole. Also found one or two minor bugs, like usages of undefined variables |
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.
... ok. It's a good idea to use common and standard formatting.
However NML uses frequently lists or items aligned for reasons that it allows a quick overview of properties or similar. Often these are also places where changes for new features are needed - so an oversight there is essential in keeping updates easy.
Yet this patch destroys informational formatting in A LOT of these places which should not be touched and in some cases must not be changed as it would make editing or reading it in an understandable way impossible. There are so many places of these type that I'm not sure that I caught each, though I made an effort.
While it certainly makes sense for many cases with quotation etc, the destruction of alignment is IMHO not acceptable as-is as it greatly reduces overall code readability.
I usually clicked the lines above the unacceptable change blocks... so below comments might seem inappropriate on their own.
Yeah, I did wonder. There's |
I'd be inclined to just go with what Black does. I've made my peace with most automated formatters (except auto-pep8). The readability advantages of non-standard formatting tend to trade off poorly against having automated code style check + formatting. An automatically enforced format also avoids nitpick code reviews where people are arguing personal or project formatting style instead of looking at whether the actual result is good. YMMV etc. |
Disagree - perhaps if it was just odd bits of nice-to-have alignment, but there are several big 'tables' of properties and other NFO-related stuff which are awkward enough to read already. |
Fixed most of these issues. Some I looked at and decided that alignment wasn't needed/necessary In a separate commit for ease of review, can squash it into the original formatting commit if desired. |
Thanks, this seems more sane, and gets rid of some pretty bizarre formatting in places. This version still de-aligns the property tables in action0properties.py, which I found hard enough to read before. That one isn't in @planetmaker's list above, so not sure if you've looked at it. The logical order would be to squash the |
Yeah, I've only looked at the tables I was specifically pointed at, feel free to review to point at more. In theory yes, adding |
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.
Maybe use # fmt: off
in nml/actions/action3_callbacks.py
7871979
to
3ffd566
Compare
067ae59
to
fcb47a0
Compare
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.
See comments, otherwise LGTM.
Based on an earlier version by Charles Pigott.
This is autogenerated by the following command: black -l 120 --exclude "(generated|nml/actions/action2var_variables.py|nml/actions/action3_callbacks.py)" nml
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 revised the Action0Properties changes to not reshuffle everything so much.
<LordAro> seems reasonable
Quite a vast amount of changes. The middle black formatting commit probably isn't worth looking at too closely, but the other two commits should be quite readable.