search

nextstrain / seasonal-cov

Nextstrain build for seasonal coronaviruses
https://nextstrain.org/seasonal-cov/
1 stars 0 forks source link

Add pre commit #10

Closed genehack closed 4 months ago

genehack commented 4 months ago

Description of proposed changes

Adds .pre-commit-config.yaml with a stock ruleset, and documentation around installing and using pre-commit to set up local git hooks to ensure commits are linter-clean.

Related issue(s)

9

genehack commented 4 months ago

Okay, I'm gonna merge this at some point tomorrow (Thursday, US time) — please provide feedback if you have any.

genehack commented 4 months ago

Is this something we want to adopt across all pathogen repos? Thoughts on adding the shared pre-commit config to the pathogen-repo-guide?

I'm +1 for standardizing on something like this, and if there's a consensus around that, I think the repo guide would be a good place for it to live.

ETA: added to lab meeting agenda as a "stretch goal" question.

tsibley commented 4 months ago

I'm +1 for standardizing on something like this [pre-commit linting and auto-formatting], and if there's a consensus around that, I think the repo guide would be a good place for it to live.

My 2¢, grain of salt, and all that, but I despise auto-formatting. Linting: no problem, it's great, more of it please. Auto-formatting: no thanks, always been a worse developer experience for me. If it's better for the group, I guess ok.

genehack commented 4 months ago

My 2¢, grain of salt, and all that, but I despise auto-formatting. Linting: no problem, it's great, more of it please. Auto-formatting: no thanks, always been a worse developer experience for me. If it's better for the group, I guess ok.

What are you thinking of as "auto-formatting", specifically? I can guess this would include the snakefmt and yamlfmt rules, probably toml-sort, but what about trailing-whitespace?

Personally, 🧂 I'm a fan of a consistent autoformat if only because it removes a whole class of PR feedback, but I can also live without if that's the consensus preference.

genehack commented 4 months ago

Okay, I'm gonna merge this at some point tomorrow (Thursday, US time) — please provide feedback if you have any.

Pausing on this plan while we continue to discuss what should be in the "standard" pre-commit; we may need another meeting session about this, including examining the semi-unstated assumption of a near-universal config...

genehack commented 4 months ago

Related issue: https://github.com/nextstrain/pathogen-repo-guide/issues/28