Revisit using pre-commit.ci

[dstansby]

If I'm makng a simple change to markdown, such as in #462, I use the GitHub web interface for speed. This can often result in pre-commit failing however, because the markdown isn't correctly linted. This then breaks the benefit of using the GitHub website for speed, because I have no way of automatically fixing the linting and have to go back to local development on my laptop.

To improve this I'd like to propose we revisit using pre-commit.ci to lint this repo. This would provide a mechanism of automatically making pre-commit fixes on demand, by posting a "pre-commit.ci autofix" comment in a PR.

In #196 we very briefly discussed and decided not to use it because it's not available for private repositories. One mitigation to this is we can still have a GitHub actions linting job, so when someone sets up a private repo from this template there's still a linting CI job that is run.

So I'd like to advocate for using pre-commit.ci for this repo. Thoughts?

[samcunliffe]

In #196 we very briefly discussed and decided not to use it because it's not available for private repositories. One mitigation to this is we can still have a GitHub actions linting job, so when someone sets up a private repo from this template there's still a linting CI job that is run.

Briefly from memory the decision to turn it off was:

1. We didn't want to have different tools in the main tooling repo than those in the template.
2. pre-commit.ci is not free for closed-source: we want to encourage maximum takeup of the template amongst UCL researchers, and a good portion of projects either start out closed until some research outcome is published, or stay closed-source.

I'm actually OK with violating 1. We could have pre-commit.ci on the repo, but not the template.

If I read you correctly, you're suggesting that we make the template a bit smarter and have it choose whether to lint based on a cookiecutter option?

[paddyroddy]

I'm happy to have it on the repo, but not the template

[dstansby]

I'm definitely only proposing for the repo here, not the template.

I'm not suggesting we alter the template - just that we add pre-commit.ci to this repo. In order to make sure we keep eating our own dog food, my suggestion is we could keep the GitHub actions linting running on this repo too. But that would duplicate linting CI on this repo 🤔 I'm not sure which way round is best...

[paddyroddy]

Installed and tested here #464 (comment)

[paddyroddy]

Sorry I'm turning this off. It is impossible to turn off autoupdates pre-commit-ci/issues#83. See the issues/concerns: #479, #482

[dstansby]

@paddyroddy could you post the issue you've found on this thread? I think they're spread around several PRs/issues at the moment, and I'm struggling to find them all 😄

[samcunliffe]

Why not taking alphas?

[paddyroddy]

Sorry I'm turning this off. It is impossible to turn off autoupdates pre-commit-ci/issues#83. See the issues/concerns: #479, #482

It's here @dstansby

[paddyroddy]

Also doesn't update the template

[dstansby]

I can follow links, but I'm asking if you could repeat the problems here so it's easier for us to see what they are all in one place 🙏

[samcunliffe]

Sorry I'm turning this off.

Hmm. I'm also a bit wary of adding and removing bots and making decisions so unilaterally.

🗳 I think we should add it, keep it on, and accept that autoupdates will come from both bots. I'm fine with alpha versions. Presumably, renovate will update the template at a slower cycle. Which is less of an issue if we are using versioning and latest.

[paddyroddy]

Hmm. I'm also a bit wary of adding and removing bots and making decisions so unilaterally.

I can understand that. But the bot did more than was advertised in #463. IMO behaviour that we don't want. The maintainer is a 🍑 and has not added the ability to turn off autoupdates.

🗳 I think we should add it, keep it on, and accept that autoupdates will come from both bots. I'm fine with alpha versions. Presumably, renovate will update the template at a slower cycle. Which is less of an issue if we are using versioning and latest.

I disagree. Reasons:

• Both the repo and template currently update at the same frequency
• We have complete control over which updates we want: major, minor, patch etc.
• We can control the frequency (I'm aware you can do this with the pre-commit bot, but ours currently aligns with GitHub Actions)
• Reduce dependency on a bot which is duplicating behaviour

If we could turn off autoupdate then I'm not opposed to having it. But it's really not hard to run pre-commit run -a on the repo.

[paddyroddy]

I can follow links, but I'm asking if you could repeat the problems here so it's easier for us to see what they are all in one place 🙏

In this thread pre-commit-ci/issues#83 there are the maintainer's reasons for not wanting the ability to turn off autoupdates.

[samcunliffe]

I disagree. Reasons:

I see.

Both the repo and template currently update at the same frequency

I'm actually not so opposed to having the repo and template out of sync. What's the worse that could happen?

Alternatives:

• Would the lite action work instead?
• Could we keep full-bodied bot and just have an action to automatically decline update PRs from pre-commit.ci?

[paddyroddy]

Not seen lite before. I think that's better than the other.

The worst I guess would be:

• Someone checks out repo (ignoring latest tag) with pre-commit out of sync
• They look into pre-commit and see the ones they have downloaded aren't the same as the one on the repo
• They update their versions to match the repo ones (potentially running the pre-commit autoupdate command they learnt about in their search)
• Their linting now fails and they don't know why

[dstansby]

I've added a new rule at https://github.com/UCL-ARC/python-tooling/settings/rules/2461978 which (I think...) bans branches called pre-commit-ci-update-config. I'm hoping this will prevent the pre-commit bot from opening auto update PRs?

@paddyroddy did you disable the bot? If so, could you re-enable it, and we can see if this works? If it does it seems like a good solution.

[samcunliffe]

@dstansby

I think we should:

• Re-enable the pre-commit.ci bot today (now).
• (Then) merge @paddyroddy's pre-commit autoupdate revert pull request (so that the pre-commit-config is back to being outdated...)
• Hold off on merging the @renovate PRs until Monday.

.... If your branch rule has worked, then there should be no autoupdate PR from pre-commit.ci despite that one is needed. On Tuesday we can merge @renovate's PRs.

[paddyroddy]

@samcunliffe I didn't do PRs in your desired order, as didn't see this. Can install bad bot now.

[paddyroddy]

Will close this. We can reopen if issues arise.