Add pre-commit hook with Black

[Molkree]

Description

• Added pre-commit to the conda environment file.
• Added pre-commit config with Black to the root. Install with pre-commit install.
• Somewhat simplified Black config by moving ignored folders to .gitignore instead.
• Added pre-commit badge to Readme.
• Updated docs.

Opening this early but I have encountered one issue. When I commit something that shouldn't pass Black it correctly gets changed. I have tested line longer than 80 chars but shorter than default 88 chars to check that pre-commit Black sees Black config. But at the same time, it still tries to modify files that it shouldn't. I have changed something inside tardis/montecarlo which should be excluded from the check but when I try to commit it formats there as well 🤔 Not sure if it's my mistake or Black/pre-commit limitation, help is appreciated here. Resolved, see comments below.

Motivation and context
This is part of #1572.

How has this been tested?

• Testing pipeline.
• Other. Tested it working on my local machine.

Type of change

• Bug fix.
• New feature.
• Breaking change.
• None of the above.

Checklist

• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    Not yet, will add it next. I guess only need to mention it in Git workflow and Code quality Black section.
  • (optional) I have built the documentation on my fork following the instructions. Link to built docs.
• I have assigned and requested two reviewers for this pull request.

[codecov]

Codecov Report

Merging #1573 (afbce29) into master (60cd602) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1573   +/-   ##
=======================================
  Coverage   61.87%   61.87%           
=======================================
  Files          63       63           
  Lines        5852     5852           
=======================================
  Hits         3621     3621           
  Misses       2231     2231           

Impacted Files	Coverage Δ
tardis/tardis/montecarlo/spectrum.py	65.51% <0.00%> (ø)
tardis/tardis/plasma/standard_plasmas.py	73.11% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60cd602...afbce29. Read the comment docs.

[epassaro]

Is this related to your issue? psf/black#1667

[Molkree]

@epassaro, no, I don't think it's related. In that issue people got confused and thought that Black config doesn't get passed when used by pre-commit. However in our case it definitely gets passed.
Simple test: in tardis you set line-length = 80 when default is 88. I just created t.py in the root with this content:

print("long", "long", "long", "long", "long", "long", "long", "long", "long", "long")

This is 85 chars long, so should be formatted according to our config. And it does get formatted when I try to commit.
However when I move this file to tardis/montecarlo which should be excluded from Black formatting, it still gets formatted when I try to commit.

So it somehow picks up line length config from .toml but ignores excluded files config 🤔

I'm just going to ask in pre-commit/Black community now.

[Molkree]

Alright, I think it is an unavoidable limitation because pre-commit basically runs Black against individual files (if you do the same with Black directly it will also format). So exclude files/dirs should be duplicated in pre-commit config for Black :/

See this comment for a similar question about flake8.

[Molkree]

By default Black excludes files in .gitignore and a bunch of others. If you use exclude it just overwrites that list with what you pass, but there's extend-exclude option that adds to that list. I've changed to this option and removed dirs that are ignored by default.

Then I just copied that explicit exclude list to pre-commit config. And added a bunch of dirs to the .gitignore so they don't get checked out.

[Molkree]

Should I split this into two PRs according to #1563?

[epassaro]

@jaladh-singhal implemented black, I think he's more qualified to review this.

[jaladh-singhal]

Everything looks great - thanks for effort @Molkree. This will surely save users from enabling black on their own but I'm not sure if it's advantageous enough that we should add pre-commit hooks for black, why:

1. If contributor's code hasn't followed black formatting, black checking workflow will fail on PR and prevent us from merging it unless it is formatted.
2. This will add pre-commit as new dependency and except black, we don't need it as of now for anything else - even for black it's optional (as mentioned in 1st point).

@wkerzendorf @andrewfullard should have final say in this.

[Molkree]

@jaladh-singhal

1. True, but moving checks earlier is good as it is.
2. Right now it's only Black but there's always potential there will be more in future (isort, flake8, etc).

This was already discussed in #1170 and I haven't seen any objections. And in #1405 you mentioned "enforcing by pre-commit hook will be in another PR" :O What has changed since January?

[Molkree]

Rebased on master to solve merge conflict in README.rst

[epassaro]

IMHO pre-commit shouldn't be a TARDIS dependency, just leave the choice of installing by yourself adding the --no-update-deps flag to conda. Also other hooks could be useful too in the future, but always optional, not enforce devs to use them.

[jaladh-singhal]

This was already discussed in #1170 and I haven't seen any objections. And in #1405 you mentioned "enforcing by pre-commit hook will be in another PR" :O What has changed since January?

@Molkree #1398 made it possible to enforce black (though not necessarily by pre-commit hook that I mentioned at that time thinking that's the only way to do it). Good reminder BTW, I've closed #1170

[tardis-bot]

Before a pull request is accepted, it must meet the following criteria:

• Is the necessary information provided?
• Is this a duplicate PR?
  • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
  • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
• Does it pass existing tests and are new tests provided if required?
  • The test coverage should not decrease, and for new features should be close to 100%.
• Is the code tidy?
  • No unnecessary print lines or code comments.

[Molkree]

#1398 made it possible to enforce black

That's true but in my opinion "please install pre-commit once if you want to contribute" is better than "please run Black before each commit if you want to contribute". Just a matter of convenience, everyone can forget to run formatter (just look at PRs, 2 out of the last 10 had commits fixing Black).

[Molkree]

just leave the choice of installing by yourself adding the --no-update-deps flag to conda

@epassaro
I'm not super familiar with conda but I think --no-update-deps will still install pre-commit, it just won't touch existing installations. So this will still add a dependency. Maybe you wanted --no-deps?

Also is this your opinion or request for change as a member of Tardis?

[andrewfullard]

This needs rebasing

[Molkree]

@andrewfullard, rebased

[Molkree]

Hm, I see there's a failing check, should I add myself to .mailmap?

[andrewfullard]

Hm, I see there's a failing check, should I add myself to .mailmap?

Yes please, that should fix the failure.

[epassaro]

We recently moved to the new Astropy package template. I want to keep their .gitignore file untouched at least for a while @Molkree

[Molkree]

@andrewfullard, mailmap fixed
@epassaro, gitignore reverted

[andrewfullard]

@epassaro I'd like your review on this.

[epassaro]

I'll review this soon 🤞

[andrewfullard]

After some discussion this has been considered unnecessary because we have the PR check in place for black and we don't want to modify developer commits without their explicit knowledge.