[fmt] Add CI step for black (and remove darker)

[RMeli]

Add black (version 24) to CI, and start formatting two files: tables.py and due.py. This showcases hot to incrementally add files to the black configuration, and how to turn off automated formatting in some portions of tables.py.

This PR also removes darken.

Developers certificate of origin

• I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

---

📚 Documentation preview 📚: https://mdanalysis--4725.org.readthedocs.build/en/4725/

[RMeli]

https://github.com/MDAnalysis/mdanalysis/actions/runs/11143944422/job/30970132772?pr=4725#step:4:79

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.58%. Comparing base (599c8db) to head (faffd70).
Report is 61 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4725      +/-   ##
===========================================
- Coverage    93.60%   93.58%   -0.03%     
===========================================
  Files          173      185      +12     
  Lines        21463    22530    +1067     
  Branches      2993     2993              
===========================================
+ Hits         20090    21084     +994     
- Misses         929     1002      +73     
  Partials       444      444              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pep8speaks]

Hello @RMeli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file package/MDAnalysis/topology/tables.py:

Line 384:80: E501 line too long (89 > 79 characters)
Line 385:80: E501 line too long (89 > 79 characters)
Line 386:80: E501 line too long (89 > 79 characters)
Line 387:80: E501 line too long (89 > 79 characters)
Line 388:80: E501 line too long (89 > 79 characters)
Line 389:80: E501 line too long (89 > 79 characters)
Line 390:80: E501 line too long (89 > 79 characters)
Line 391:80: E501 line too long (89 > 79 characters)
Line 392:80: E501 line too long (89 > 79 characters)
Line 393:80: E501 line too long (88 > 79 characters)
Line 394:80: E501 line too long (89 > 79 characters)
Line 395:80: E501 line too long (89 > 79 characters)
Line 396:80: E501 line too long (89 > 79 characters)
Line 397:80: E501 line too long (85 > 79 characters)
Line 398:80: E501 line too long (86 > 79 characters)

Comment last updated at 2024-10-14 19:23:05 UTC

[RMeli]

Here the automated formatting is off, but I manually removed trailing whitespaces (hence the diff).

[RMeli]

Let's start simple, with only two files. Other files or matching regexp can be added with the | operator.

[RMeli]

https://github.com/MDAnalysis/mdanalysis/actions/runs/11151800405/job/30996028106?pr=4725#step:4:69

[IAlibay]

lgtm!

[RMeli]

This PR now contains only 3 commits, and should not be squash-merged:

• ee6cbd1 enables black in CI and cleans up darker
• feca121 formats due.py and tables.py
• ae05db7 adds the previous commit to .git-blame-ignore-revs, so that git blame will point to the original author

The latter is the reason why the PR should not be squash-merged.

[orbeckst]

@IAlibay can you please shepherd the PR as you're a reviewer and it changes the CI?

[orbeckst]

@RMeli can you please resolve conflicts?

[RMeli]

@orbeckst I don't see the conflicts, two are files being deleted... I think GitHub might have been confused by my mess with my rebasing + cherry piking commits + force pushing.

[orbeckst]

This is what it looks like to me:

At least I couldn't merge from the GH interface.

[RMeli]

Yes, it looks the same for me, but locally I was unable to see any conflict. I think I also rebased develop, but I'll try again later. Worst case I'll close this PR and open a new one with the commits cherry picked.

[RMeli]

I really don't know what is going on here... I just created a new branch from develop, cherry picked the three commits mentioned above, hard-reset this branch to the new branch, and force-pushed.

If I rebase develop, it says this is up to date. Same with merge.

[IAlibay]

@RMeli I fixed the merge conflict, but that added two commits - can you check if the commit history needs cleaning up or if it's still good to non-squash merge?

[RMeli]

Thanks @IAlibay. I see a merge commit from develop (strange, for me it did not work…), which would be better not to have in the develop branch. I’m try to squash a few commits.

[IAlibay]

which would be better not to have in the develop branch

I mean for the odd occasion I think we can live with it.

[RMeli]

@IAlibay I rebased on develop and this time it seemed to work alright. It should be good to go if CI comes back green. Thanks for the help!

[RMeli]

The ignored commit was wrong, I fixed that and squashed down to two commits.

[RMeli]

This workflow is not great IMO, it requires a lot of git-foo. I suggest that going forward I take note of the different black PRs that are squash-merged (maybe in an issue), and then periodically open a PR to add them to .git-blame-ignore-revs.

[orbeckst]

If we agree to use a common title for black-only commits or add a magic comment then we could (semi)automate adding to the exclusion file.

[RMeli]

Excellent suggestion @orbeckst. I suggest we use [fmt] as tag for formatting-related PRs.