Skip to content
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

Merged
merged 9 commits into from Nov 7, 2020
Merged

Blacken & Flake8 nml #103

merged 9 commits into from Nov 7, 2020

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Apr 23, 2020

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.

@LordAro LordAro force-pushed the blacken branch 2 times, most recently from 646c809 to d63415c Compare April 23, 2020 23:42
@andythenorth
Copy link
Contributor

I did start reading the Black commit, obviously got bored, but I think it's great.

Hopefully we can get this approved soon :)

@planetmaker
Copy link
Contributor

Can you give me a brief explanation of what problem this solves?

@LordAro
Copy link
Member Author

LordAro commented Apr 24, 2020

Consistent formatting amongst NML itself, and OpenTTD python as a whole.

Also found one or two minor bugs, like usages of undefined variables

Copy link
Contributor

@planetmaker planetmaker left a 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.

nml/actions/action0.py Show resolved Hide resolved
nml/actions/action2random.py Show resolved Hide resolved
nml/actions/action2var_variables.py Outdated Show resolved Hide resolved
nml/actions/action2var_variables.py Outdated Show resolved Hide resolved
nml/actions/action2var_variables.py Outdated Show resolved Hide resolved
nml/grfstrings.py Outdated Show resolved Hide resolved
nml/palette.py Show resolved Hide resolved
nml/parser.py Show resolved Hide resolved
nml/tokens.py Show resolved Hide resolved
nml/tokens.py Show resolved Hide resolved
@LordAro
Copy link
Member Author

LordAro commented Apr 24, 2020

Yeah, I did wonder. There's # fmt: off & # fmt: on blocks that can be applied. I'll have a look.

@andythenorth
Copy link
Contributor

andythenorth commented Apr 24, 2020

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.

@FLHerne
Copy link
Contributor

FLHerne commented Apr 24, 2020

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.

@LordAro
Copy link
Member Author

LordAro commented Apr 25, 2020

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.

@FLHerne
Copy link
Contributor

FLHerne commented Apr 25, 2020

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 # fmt: off comments and other config changes into the first commit, and then rerun the huge automated change based on that?

@LordAro
Copy link
Member Author

LordAro commented Apr 25, 2020

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 # fmt would be in the first commit, but in practice i'm still doing a load of manual formatting to make it closer to "good", which I absolutely do not want to repeat (space space space space space...)

Copy link
Contributor

@glx22 glx22 left a 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

nml/actions/action0.py Outdated Show resolved Hide resolved
nml/actions/action2var_variables.py Outdated Show resolved Hide resolved
nml/actions/action2var_variables.py Outdated Show resolved Hide resolved
nml/actions/real_sprite.py Show resolved Hide resolved
nml/global_constants.py Show resolved Hide resolved
@LordAro LordAro force-pushed the blacken branch 8 times, most recently from 7871979 to 3ffd566 Compare October 3, 2020 15:55
@LordAro LordAro force-pushed the blacken branch 3 times, most recently from 067ae59 to fcb47a0 Compare October 17, 2020 15:28
Copy link
Contributor

@FLHerne FLHerne left a 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.

nml/actions/real_sprite.py Outdated Show resolved Hide resolved
nml/tokens.py Outdated Show resolved Hide resolved
nml/actions/action0properties.py Show resolved Hide resolved
FLHerne and others added 4 commits November 7, 2020 18:28
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
Copy link
Contributor

@FLHerne FLHerne left a 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

@FLHerne FLHerne merged commit 72bc37a into OpenTTD:master Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants