Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add pre commit #10

Merged
merged 8 commits into from
May 29, 2024
Merged

Add pre commit #10

merged 8 commits into from
May 29, 2024

Conversation

genehack
Copy link
Contributor

@genehack genehack commented May 20, 2024 •
edited
Loading

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 genehack requested a review from a team May 21, 2024 17:13
@genehack genehack force-pushed the add-pre-commit-9 branch from b4c8acf to 16deee9 Compare May 21, 2024 19:34
@genehack genehack force-pushed the add-pre-commit-9 branch 2 times, most recently from da81c45 to 3e5825e Compare May 22, 2024 16:28
@genehack genehack requested a review from a team May 23, 2024 02:41
@genehack
Copy link
Contributor Author

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

joverlee521
joverlee521 reviewed May 23, 2024
View reviewed changes
Copy link

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@genehack
Copy link
Contributor Author

genehack commented May 23, 2024 •
edited
Loading

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.

victorlin
victorlin requested changes May 23, 2024
View reviewed changes
ingest/config/defaults.yaml Outdated Show resolved Hide resolved
@genehack genehack force-pushed the add-pre-commit-9 branch from 659fe5c to 767db8d Compare May 23, 2024 17:40
@tsibley
Copy link
Member

tsibley commented May 23, 2024

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.

@joverlee521 joverlee521 mentioned this pull request May 23, 2024
Merged
@genehack
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Related issue: nextstrain/pathogen-repo-guide#28

@genehack
Add pre-commit GitHub Action [#9]
3d9c35f 
Keeping things self-contained to Github Actions seems better than
introducing a dependency on another third-party service, particularly
when the main advantage of that third-party service seems to be
automatically correcting things, which is not a feature I'm interested
in — this is intended only as a compliance check.
@genehack genehack force-pushed the add-pre-commit-9 branch from 86df70f to 3e66f86 Compare May 29, 2024 16:52
@genehack
Strip down pre-commit config [#9]
c5946b4 
* remove snakefmt
* remove yamlfmt
* remove codespell
* remove toml-sort
* remove associated configuration

All of these are things that are potentially "auto-formatting" code.
@genehack genehack force-pushed the add-pre-commit-9 branch from 3e66f86 to c5946b4 Compare May 29, 2024 16:54
@genehack genehack merged commit 3c6b27d into main May 29, 2024
2 checks passed
@genehack genehack deleted the add-pre-commit-9 branch May 29, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joverlee521 joverlee521 joverlee521 left review comments

@tsibley tsibley tsibley left review comments

@victorlin victorlin victorlin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@genehack @tsibley @victorlin @joverlee521