Open-EO / openeo-python-client

Python client API for OpenEO
https://open-eo.github.io/openeo-python-client/
Apache License 2.0
156 stars 42 forks source link

Apply pre-commit + black to all files #525

Closed theroggy closed 3 months ago

theroggy commented 10 months ago

While making the changes in https://github.com/Open-EO/openeo-python-client/pull/524, they were mixed with changes made by black because black hasn't been applied yet on all files.

I think it would be more practical to apply this in one commit (and add this commit to .git-blame-ignore-revs)?

soxofaan commented 10 months ago

While making the changes in https://github.com/Open-EO/openeo-python-client/pull/524, they were mixed with changes made by black

This is a bit intended because we currently use darker instead of black to gradually apply the black code style to the code base by piggybacking changed lines/regions from normal commits, instead of doing it all at once on the whole code base.

Doing it all at once is quite a big change. Even though you can alleviate the blame problem with .git-blame-ignore-revs, you still break (probably) all open PRs for example.

Personally, I'm not against pushing the black code style (or a subset) more aggressively, but this has to be supported by the main contributors of this repo.

soxofaan commented 10 months ago

Also note that there are some parts of the openeo python client code base that are programmatically generated (e.g. "openeo/processes.py"). It takes more effort to make this setup black compatible than just running the black tool on that).

(I think that's the reason of the failed test of this pull request)

theroggy commented 10 months ago

Also note that there are some parts of the openeo python client code base that are programmatically generated (e.g. "openeo/processes.py"). It takes more effort to make this setup black compatible than just running the black tool on that).

(I think that's the reason of the failed test of this pull request)

Ah, OK. I'm not able to see the errors, I get a ERR_CONNECTION_TIMED_OUT on https://[jenkins.vgt.vito.be](https://jenkins.vgt.vito.be/job/openEO/job/openeo-python-client/job/PR-525/2/display/redirect)/job/openEO/job/openeo-python-client/job/PR-525/2/display/redirect

theroggy commented 10 months ago

While making the changes in #524, they were mixed with changes made by black

This is a bit intended because we currently use darker instead of black to gradually apply the black code style to the code base by piggybacking changed lines/regions from normal commits, instead of doing it all at once on the whole code base.

OK, I typically try to avoid mixed commits of "functional" code changes versus commits to fix "style", but I suppose that's just a habit.

Doing it all at once is quite a big change. Even though you can alleviate the blame problem with .git-blame-ignore-revs, you still break (probably) all open PRs for example.

I don't recall having a lot of issues because of this when black is also applied on the open PR's, but it might indeed give issues.

Personally, I'm not against pushing the black code style (or a subset) more aggressively, but this has to be supported by the main contributors of this repo.

Definitely!

soxofaan commented 10 months ago

Ah, OK. I'm not able to see the errors, I get a ERR_CONNECTION_TIMED_OUT on http

indeed that's an internal CI system. But I now just also enabled the Github actions to run and they fail on the same thing

I typically try to avoid mixed commits of "functional" code changes versus commits to fix "style",

That is indeed also a valid point. However, even without a tool like black I often tend to make minor formatting improvements on lines that are touched anyway, as long as it doesn't ruin the signal-noise ratio too much.

I don't recall having a lot of issues because of this when black is also applied on the open PR's,

I'm afraid each PR owner will be forced to handle confusing merge conflicts when master is completely reformatted. Even if they apply black on their branch before merging, I think git will still panic about conflicts all over the place. That's why I'm a bit hesitant to fully go all-in on black with a single commit.

soxofaan commented 3 months ago

I don't think it's useful to keep this PR still open. It doesn't apply anymore and it can easily be redone by running the necessary tools (which I'm planning to do anyways).