adding pre-commit as a dep

[jandom]

This PR adds pre-commit as a dependency including a simple configuration file. A number of "hooks" are enabled, mostly around stripping stray whitespaces.

This PR is broken down version of this PR https://github.com/MDAnalysis/UserGuide/pull/270

[mikemhenry]

You can use this to help fix git blame on the gihub web UI https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

[jandom]

Ah, now I understand how this feature will work! Would it make sense to create this file in this repo, after all the PRs are merged, if only as an exercise?

[mikemhenry]

I don't know exactly how MDAnalysis does things and I don't want to make @IAlibay @richardjgowers or @hmacdope grumpy, but what I would do is:

• Make all the commits you need
• Squash them into a single commit, note the hash
• Make another commit with .git-blame-ignore-revs that has that commit in it
• Merge those two commits in

That way you don't have to add a bunch of commits into that file (not a huge deal, but it makes it easy to annotate the file with the PR or something in a comment) and keeps history clean.

[IAlibay]

I don't know exactly how MDAnalysis does things and I don't want to make @IAlibay @richardjgowers or @hmacdope grumpy, but what I would do is:

• Make all the commits you need
• Squash them into a single commit, note the hash
• Make another commit with .git-blame-ignore-revs that has that commit in it
• Merge those two commits in

That way you don't have to add a bunch of commits into that file (not a huge deal, but it makes it easy to annotate the file with the PR or something in a comment) and keeps history clean.

Yeah that's also what I'd suggest doing.

[RMeli]

Do the pre-commit checks need to be added in CI as well? IIRC, users need to add the pre-commit hooks, so we might want to check that they did run.

[mikemhenry]

Another solution is https://pre-commit.ci/ which will run the hooks and commit the results. That way devs don't need to worry about it

I think that if you don't use the pre-commit.ci thing, you need to add the pre-commit hooks to CI to make sure they get ran. The nice thing is that once everyone has it installed, the pre-commit hooks will keep code that doesn't pass from ever getting committed (if ran locally).

[jandom]

Many thanks for the pre-commit.ci @mikemhenry, it works great and was super easy to install – it's added here now but obviously fails.

Edit: locally pre-commit can be configured to run a variety of tools (black, isort). pre-commit.ci will never support these, because an installation is required, pre-commit.ci only runs the vanilla pre-commit hooks (striping spaces and such). pre-commit.ci "lite" does support other code modification tools (but needs to be part of a GH actions workflow, see https://pre-commit.ci/lite.html)

[RMeli]

Thanks @mikemhenry for the suggestions.

The nice thing is that once everyone has it installed, the pre-commit hooks will keep code that doesn't pass from ever getting committed (if ran locally)

Indeed. My main concern is forgetting to install them locally, that's why I think they should run in CI too.

I don't know how I feel about actions adding automated commits, but in this case it might be useful.

locally pre-commit can be configured to run a variety of tools (black, isort). pre-commit.ci will never support these

This might not be a concern if the other tools are added to CI too.

What I was thinking was to do something like "pre-commit run --all-files" in CI to check that pre-commit hooks were installed (and run) locally. But automation might reduce the burden on developers, so it might be worth exploring.

[jandom]

Sure @RMeli, let me re-write it as a vanilla GH action – shouldn't take much work

[jandom]

@RMeli have a look now please! :)

[jandom]

@RMeli @IAlibay PTAL, i think we've reviewed this PR extensively – the CI check is failing until the automatic code changes are applied in this PR https://github.com/MDAnalysis/UserGuide/pull/272

[jandom]

@RMeli @IAlibay what do you guys think? would love to move ahead with this PR, either further revisions, or a merge

[RMeli]

Sorry @jandom, I've been really swamped this week. LGTM, so I think it's time to merge #272 so that CI tests can pass.

[IAlibay]

Sorry @jandom thanks for pushing this forward, unfortunately we've got CI/CD issues in core and those are at the top of my priority list.

@MDAnalysis/coredevs can someone else jump in here to take over review? Seems like both @RMeli and myself are pretty swamped atm.

[RMeli]

Blocked by #275? The CI failure is the same as the one described in one of the comments there.

https://github.com/MDAnalysis/UserGuide/pull/275#issuecomment-1650444458

[jandom]

Blocked by https://github.com/MDAnalysis/UserGuide/pull/275? The CI failure is the same as the one described in one of the comments there.

Well, not really – development already broken with the TNG error https://github.com/MDAnalysis/UserGuide/commit/3f423ba001011aed76a1a10ba3a0701fd2d1d8f2

I'd suggest for a merge of this PR, since it doesn't introduce any new error, and resolve #275 in parallel

[RMeli]

I did not realise that #272 was not targeting the default branch, so I was a bit confused as of why all the changes of that PR re-appear here. Any reason for this? I was under the impression that we wanted to merge #272 into the default branch and then review/check this smaller PR with the config and CI.

[RMeli]

Basically it seems we are back to #270.

[jandom]

Yeah, I think we both got confused here

Sorry @jandom, I've been really swamped this week. LGTM, so I think it's time to merge https://github.com/MDAnalysis/UserGuide/pull/272 so that CI tests can pass.

I thought you wanted to merge the auto-changes merged into this branch, rather than development, to get CI green :D

[jandom]

I can attempt a github/git surgery and revert a few things, but would prefer to either push ahead, or rebase and split into two commits (one manual, one automated)

[RMeli]

Sorry for the confusion! I think pushing ahead would require ignoring both your changes (config and CI) and the automatic changes for git blame.

It might be worth merging #272 to the main branch and then merge this PR, but I'm OK to go ahead with this one too if it's easier. I don't mind too much. @IAlibay do you have strong preferences?

[jandom]

I've re-created #272 as a new PR https://github.com/MDAnalysis/UserGuide/pull/281, hopefully that'll make things simpler

[IAlibay]

Sorry for the confusion! I think pushing ahead would require ignoring both your changes (config and CI) and the automatic changes for git blame.

It might be worth merging #272 to the main branch and then merge this PR, but I'm OK to go ahead with this one too if it's easier. I don't mind too much. @IAlibay do you have strong preferences?

Not really, seems like all paths will lead to the same thing, please go ahead.

[jandom]

Not really, seems like all paths will lead to the same thing, please go ahead.

I'm going to interpret this as approval and merge away :-)

[RMeli]

I think we needed to add 87210209ab00ad7257fa7df2c06e8b4caa579e26 to .git-blame-ignore-revs (https://github.com/MDAnalysis/UserGuide/pull/271#issuecomment-1645750807)?

[RMeli]

But thanks for all the work @jandom!

[jandom]

Thanks for all the reviews – I hope this will be a nice demo!