Update black to 22.3.0 and ditch the deprecated pre-commit Action

[santisoler]

Make pre-commit to use the latest stable version of Black: 22.3.0. Delete the pre-commit.yaml Action workflow. Replace the deprecated pre-commit/action for installing and running Black in the linting.yaml Action workflow. Reformat xrft code with the latest Black stable release.

Fix #183

[rabernat]

Thanks for this contribution @santisoler!

In order to get our linting checks to pass, you also have to update

https://github.com/xgcm/xrft/blob/42abb0fd0474f891cab84e0ede86d7f18c16b8b4/.pre-commit-config.yaml#L1-L6

@jbusecke and @roxyboy, can you explain why we have two liniting github workflows (code-style and pre-commit)?

[roxyboy]

@jbusecke and @roxyboy, can you explain why we have two liniting github workflows (code-style and pre-commit)?

This probably has to do with legacy configuration. I think the two linting workflows are coming from .github/workfows and pre-commit-config.yml. I don't know which one is the more up-to-date best practice... @jbusecke Do you have any take on this?

[jbusecke]

As far as i can tell one was introduced by #120 and then the second one was added in #165. I think this was just an oversight? I dont see a reason to have both. They are functionally the same as far as I can see with one exception: linting.yaml does run on PRs, which is helpful IMO.

I also just checked the branch protection rules and saw that there are no required checks! Once these two here are consolidated I recommend setting the linting + core CI actions as required tests to be merged.

[roxyboy]

As far as i can tell one was introduced by #120 and then the second one was added in #165. I think this was just an oversight? I dont see a reason to have both. They are functionally the same as far as I can see with one exception: linting.yaml does run on PRs, which is helpful IMO.

So... would the simple fix to be to simply git rm pre-commit-config.yml?

[jbusecke]

yip that would work. Not sure if you want to handle that in this PR or in a separate one though. Happy to set the branch protections if you want.

[roxyboy]

yip that would work. Not sure if you want to handle that in this PR or in a separate one though. Happy to set the branch protections if you want.

This PR is about making Black formatting work smoothly so I think it would be ok to delete it in this PR. Not sure what branch protection is but that sounds like a good idea. Please do @jbusecke :)

[santisoler]

After I created this PR I found there are two issues with black.

1. The first one is the one mentioned here: Black stable version has changed some style rules, therefore the old code will fail against it.
2. The second one is described in #183 (the pre-commit action has been deprecated, therefore it fails to run in Actions).

If you agree I can solve both issues in this PR.

[roxyboy]

After I created this PR I found there are two issues with black.

1. The first one is the one mentioned here: Black stable version has changed some style rules, therefore the old code will fail against it.
2. The second one is described in Pre-commit GitHub Action has been deprecated #183 (the pre-commit action has been deprecated, therefore it fails to run in Actions).

If you agree I can solve both issues in this PR.

Yes please to both :) Thanks @santisoler

[santisoler]

Since pre-commit will always use Black 22.3.0, do you want to also pin Black on the ci/environment-py*.yml files?

[jbusecke]

Actually I think we should remove black from those files. Users should always go through pre-commit run --all-files which will ensure the black versions between local/ci environments are the same.

[jbusecke]

I have just refactored the CI environments in #184, and also removed black from there. If those tests pass, you could rebase this on on top of #184 EDIT: They pass (and I added some more recent python versions to the testing).

[jbusecke]

The second one is described in https://github.com/xgcm/xrft/issues/183 (the pre-commit action has been deprecated, therefore it fails to run in Actions).

Oh wow @santisoler thanks for making me aware of this! Ill have to put in some work to update all my repos haha.

[jbusecke]

Happy to set the branch protections if you want.

I just set those, and require all python versions to pass (and also disallow pushes to main). These can be overwritten by admins (but you will get a nice red warning before).

[jbusecke]

Actually I think we should remove black from those files. Users should always go through pre-commit run --all-files which will ensure the black versions between local/ci environments are the same.

It might be best to mention this in the docs, similarly to what we have in xgcm

[roxyboy]

Actually I think we should remove black from those files. Users should always go through pre-commit run --all-files which will ensure the black versions between local/ci environments are the same.

It might be best to mention this in the docs, similarly to what we have in xgcm

It seems that it's already mentioned here...? In any case, I feel like this PR is ready to merge?

[jbusecke]

Ah excellent, thanks for pointing that out @roxyboy. Let me rebase this and then when the tests pass this is good to go from my end.

[roxyboy]

Ah excellent, thanks for pointing that out @roxyboy. Let me rebase this and then when the tests pass this is good to go from my end.

Cool, thanks for your edits @jbusecke :) Do you agree @santisoler that this PR is ready to merge?

[santisoler]

Cool, thanks for your edits @jbusecke :) Do you agree @santisoler that this PR is ready to merge?

Sure! Feel free to take any decision you feel the best regarding pinning or unpinning Black and merge it right away.