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

Reapply black without any options #560

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

HuidaeCho
Copy link
Member

@HuidaeCho HuidaeCho commented Jun 22, 2021

Personally, I think this black formatting breaks my semantic indentation and even violates PEP 8. Also, it's not compatible with vim's gq and gw with the default line width. See this and this.

@HuidaeCho HuidaeCho merged commit b72a782 into OSGeo:master Jun 22, 2021
@HuidaeCho HuidaeCho deleted the g_projpicker_black branch June 22, 2021 16:06
@HuidaeCho HuidaeCho restored the g_projpicker_black branch June 22, 2021 16:06
@HuidaeCho HuidaeCho deleted the g_projpicker_black branch June 22, 2021 16:06
@wenzeslaus
Copy link
Member

Since this breaks CI, please, either revert this PR or introduce a separate settings for this code after opening a discussion on mailing list about exceptions for individual addons.

I hope you forgive me that don't want to comment on it further here as this is not the a place to discuss the style. The place to do that was OSGeo/grass#543 as also stated in OSGeo/grass#766 (comment) and also on grass-dev in February-April and June 1 threads.

@HuidaeCho
Copy link
Member Author

HuidaeCho commented Jun 23, 2021

Since this breaks CI

Does this PR break CI? Interesting. I thought CI just does black with no --line-length? Let me check... So should I use just black or add any arguments? OK, I was aliasing black to black -l 79, my bad. Does it mean we're forced to apply black with no arguments to not break CI?

I hope you forgive me that don't want to comment on it further here

No need to comment on this here. I already expressed my opinion on this topic here too late. Just surprised that the black formatting of this script was changed to something else (without -l 79? I tried to figure that out).

@HuidaeCho
Copy link
Member Author

@wenzeslaus
Copy link
Member

I thought CI just does black with no --line-length? Let me check... So should I use just black or add any arguments?

There is a configuration file Black is using (it is using generic pyproject.toml), so yes, using no arguments is correct. It reads config from the config file. Most of these linting and formatting tools behave in the way that command line arguments override config file values which is what happened in you case. The change in g.projpicker files was caused by me applying a consistent Black with the settings from core in the whole addons repo.

OK, I was aliasing black to black -l 79, my bad. Does it mean we're forced to apply black with no arguments to not break CI?

Each project using Black would generally use either the Black defaults or have a config file, so you should run black without arguments every time. Unrelated to GRASS, if you want something else for your own projects, the right way is to create a config file. You can setup another alias (black79, ha ha!) to format your projects without a config file, but that makes it more difficult to share with other people.

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

2 participants