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

Open
wants to merge 9 commits into
base: master
from
Open

Blacken & Flake8 nml #103

wants to merge 9 commits into from

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 LordAro:blacken branch 2 times, most recently from 646c809 to d63415c Apr 23, 2020
@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Apr 24, 2020

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

@planetmaker planetmaker commented Apr 24, 2020

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

@LordAro
Copy link
Member Author

@LordAro 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

... 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 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 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 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 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 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 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

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 LordAro:blacken branch 8 times, most recently from 7871979 to 3ffd566 Oct 1, 2020
@LordAro LordAro force-pushed the LordAro:blacken branch 3 times, most recently from 067ae59 to fcb47a0 Oct 17, 2020
Copy link
Contributor

@FLHerne FLHerne left a comment

See comments, otherwise LGTM.

nml/actions/real_sprite.py Outdated Show resolved Hide resolved
nml/tokens.py Outdated Show resolved Hide resolved
'introduction_date': {'size': 4, 'num': 0x17},
'sort_order': {'size': 1, 'num': 0x1A},
'name': {'size': 2, 'num': 0x1B, 'string': 0xDC},
'maintenance_cost': {'size': 2, 'num': 0x1C},
}

This comment has been minimized.

@FLHerne

FLHerne Oct 17, 2020
Contributor

I think it's a mistake to move this away from the road/rail/tramtype blocks it's used by -- it's basically part of those, and anyone reading the code will want to see both at the same time.

[for context, each of those blocks contained a copy-pasted version of this until I factored it out recently-ish]

It's also now above the comment that explains what it means.

This comment has been minimized.

@FLHerne

FLHerne Oct 17, 2020
Contributor

Oh, but properties now being a huge list literal means you can't do that easily.

To be honest, I'm not convinced this commit is an improvement -- a list with arbitrary 'None' holes isn't clearer than the dict, especially when you have to put the indices in comments anyway because they're semantically meaningful.

Ideally we should use the feature names instead of the hex values, I think there's a patch for that in one of @planetmaker 's closed PRs.

This comment has been minimized.

@LordAro

LordAro Oct 17, 2020
Author Member

You could be right. The intention was to remove the number of #fmt: off/on comments. As TB has said, perhaps the best solution longterm is to move all of these large tables of data into a separate folder that can be properly excluded from checks, as data is not code (like was done for eints)

@LordAro LordAro force-pushed the LordAro:blacken branch from fcb47a0 to 5859ba2 Oct 17, 2020
@LordAro LordAro force-pushed the LordAro:blacken branch from 5859ba2 to 5544daf Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.