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

Question: feasibility of Black-formatting the code in this repo? #222

Open
ehrenfeu opened this issue Jun 21, 2024 · 9 comments
Open

Question: feasibility of Black-formatting the code in this repo? #222

ehrenfeu opened this issue Jun 21, 2024 · 9 comments

Comments

@ehrenfeu
Copy link
Contributor

Hi all,

this is "just" a request of your feelings in regards of running the Black formatter on the Python files in this repo?

We've been using it for some years now on our code and really like the benefits (particularly having very readable code and minimal diffs, independently of who's coding). Many recent development tools (VS Code, PyCharm, ...) are offering an integration for it, meaning they will also run the formatter whenever a file is being saved.

Having the scripts formatted by Black would IMHO facilitate interaction quite a bit (e.g. currently I would always have to explicitly ask VS Code to save the file without running the formatter in order to prevent it from producing a ton of changes). I'm even tempted to say this would make it more attractive for potential contributors, but that's obviously my very personal and strongly biased opinion 😁 😉

Now, obviously, I could just do this and file a PR - however, this would trick git and Github into believing that I am a major contributor to this repo as all the (black) changed lines would be attributed to my account. Therefore, even if you were positive about my suggestion / question, I would prefer someone else from the actual contributors to perform the change.

Happy to hear about your opinion. And just to be clear, a plain "no 🙅🏼‍♂️ we don't want this" is obviously fully acceptable!

Cheers,
Niko

@will-moore
Copy link
Member

Sounds good. 👍
I guess you want this checked in the github actions for open PRs, to make sure the formatting is maintained?

@jburel
Copy link
Member

jburel commented Jul 4, 2024

Before running black, any PR will need to be integrated. This will have a major impact on old code and will need to be discussed with active contributors like @Tom-TBT. This is why we do not run it on repository like omero-py.

@Tom-TBT
Copy link

Tom-TBT commented Jul 4, 2024

Hello,
I am ok with running black too. I can help with reviewing the open PRs that are not mine.

@sbesson
Copy link
Member

sbesson commented Jul 4, 2024

In general, I am fairly supportive of any tools that improve code readability and quality and support the writing, review and inclusion of code contributions. I believe this was the spirit of introducing flake8 to the OMERO Python codebase many years ago.

On black, adding a few thoughts based on the experience of formatting the OMERO.web repository in ome/omero-web#218. @jburel has raised the operational impact on open contributions which typically need to be coordinated, put on hold and updated to fix conflicts. Post-formatting, it is also worth mentioning the impact on code maintenance. Running an existing repository through black can easily modify 1-10K lines of code depending on the size, also splitting lines to comply with the formatting rules. This introduces noise, clutters the version history and increases the complexity of any troubleshooting/maintenance task that requires to look at historical changes in order to track the introduction of a particular feature/bug.

Overall my reluctance has more to do with the scenario of formatting an existing codebase with significant history. For any new Python repository, introducing a formatting style does not carry the downsides mentioned above.

@chris-allan
Copy link
Member

What @sbesson said.

As someone who is often trying to answer the question "why did we do this 10 years ago?" when pursuing low level bugfixes the history churn created by sweeping formatting changes makes that a lot more difficult. black is particularly bad for this because it is very opinionated on how it handles multi-line blocks and their relationship to the maximum line length.

@joshmoore
Copy link
Member

Two quick points:

@chris-allan
Copy link
Member

Thanks, @joshmoore. I need to update my git version apparently; you need at least 2.23.0 to use .git-blame-ignore-revs. Might give it a try on omero-web where there is a lot of historical reformatting mess in the history just to see how it behaves.

@sbesson
Copy link
Member

sbesson commented Jul 4, 2024

Very interesting indeed and it looks like the feature is already integrated in the GitHub UI - https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

@joshmoore
Copy link
Member

An example from the zarr-python repo: zarr-developers/zarr-python#1459

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

No branches or pull requests

7 participants