materialsproject / atomate2

atomate2 is a library of computational materials science workflows
https://materialsproject.github.io/atomate2/
Other
167 stars 96 forks source link

Should we enable pre-commit CI? #998

Closed Andrew-S-Rosen closed 1 month ago

Andrew-S-Rosen commented 1 month ago

For whatever reason, the pre-commit CI does not appear to be working on PRs right now. For instance, see the first commit in https://github.com/materialsproject/atomate2/pull/996.

janosh commented 1 month ago

i think there's a misunderstanding. this repo has a workflow to run pre-commit as a GitHub action. it does not use https://pre-commit.ci which is what offers auto-fixing lint issues.

Andrew-S-Rosen commented 1 month ago

@janosh: Oh, I thought all the materialsproject repos had pre-commit CI activated. My mistake.

So, let me rephrase my original post: why don't we have it here? It's so useful.

janosh commented 1 month ago

up to @utf

fwiw, i hope uv decides to in-house git hook installation (essentially a rust-native implementation of https://github.com/tox-dev/pre-commit-uv) at which point i would ditch pre-commit on my own repos

utf commented 1 month ago

I've never been a fan of the GitHub action. It promotes more GitHub pushes (each generally only fixing aspects of the failing linting) and hence more emails for me to read. I'd much rather people install the precommit locally as detailed in the developer docs.

That said, if I'm heavily outweighed on this, I'd consider enabling it.

janosh commented 1 month ago

It promotes more GitHub pushes (each generally only fixing aspects of the failing linting)

true, but that's why we squash merge 😄

That said, if I'm heavily outweighed on this, I'd consider enabling it.

i'm ambivalent

Andrew-S-Rosen commented 1 month ago

and hence more emails for me to read

I am shocked you have emails enabled. 😅 I wouldn't dare...

I personally find it extremely convenient and rely on it heavily, but of course I have no problem deferring to your preference on this @utf! I was mostly surprised since I had incorrectly thought it wasn't working when in reality it's not installed here.

I'll close this issue given @utf's preference. We can re-open if others have strong opinions besides just me.

JaGeo commented 1 month ago

I always find it useful but I am def guilty of many commits so I can live with both solutions.