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.

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

add formatting hook #3793

Closed
wants to merge 1 commit into from
Closed

add formatting hook #3793

wants to merge 1 commit into from

Conversation

Oblarg
Copy link
Contributor

@Oblarg Oblarg commented Dec 17, 2021

Adds formatting hook to automatically wpiformat any staged files on commit. Can be installed by running npm install .

@Oblarg Oblarg requested a review from a team as a code owner December 17, 2021 20:24
@ThadHouse
Copy link
Member

I am 100% not a fan of this. Npm is a plague that should be avoided at all costs. It also seems extremely niche. I don't want to be more responsible for npm and dependency bugs when they occur like they always do.

@Oblarg
Copy link
Contributor Author

Oblarg commented Dec 17, 2021

It's a dev dependency, whose only purpose is to dump some stuff into git hooks once. This particular toolchain is extremely popular and well-maintained, and if you freeze the versions there's really nothing to worry about.

If you don't like it, you can just ignore it; it doesn't interact with the rest of the code in any way.

@calcmogul
Copy link
Member

It's a dev dependency, whose only purpose is to dump some stuff into git hooks once

Couldn't we do the same thing with a few lines of bash/bat script instead of thousands of lines of Node stuff?

@Oblarg
Copy link
Contributor Author

Oblarg commented Dec 17, 2021

you'd have to essentially reimplement lint-staged, which isn't impossible but also isn't trivial - the nifty thing about lint-staged is that it correctly handles the formatting entirely in the git staging area

@calcmogul
Copy link
Member

calcmogul commented Dec 17, 2021

I'd rather do that. It's not hard in principle: when the commit hook is called, you get a list of staged files (there's a git plumbing command for that), call wpiformat -f with the staged filenames, then re-add the same filenames to the staging area.

@Oblarg
Copy link
Contributor Author

Oblarg commented Dec 17, 2021

relevant: diffplug/spotless#791 (comment)

@Oblarg
Copy link
Contributor Author

Oblarg commented Dec 17, 2021

perhaps we could build this into wpiformat?

@calcmogul
Copy link
Member

calcmogul commented Dec 17, 2021

Commit hooks are not wpiformat's responsibility. The commit hook setup should be done in a separate script. It's not hard to add hooks manually either, so I'm not sure even the setup should be automated.

Here's an example pre-commit hook: https://blog.jdriven.com/2020/11/formatting-in-pre-commit-hook/

@Oblarg
Copy link
Contributor Author

Oblarg commented Dec 17, 2021

That example doesn't handle partially-staged files correctly, which is basically the only part of lint-staged that isn't trivial.

@calcmogul
Copy link
Member

calcmogul commented Dec 17, 2021

I don't know why you'd ever want to only partially run the formatter on a file, but OK. Stash the unstaged changes, run wpiformat on the staged files, add them to the staging area, then unstash the unstaged files at the end. Still not hard if you know the git one-liners to do those things.

@calcmogul
Copy link
Member

calcmogul commented Dec 17, 2021

This PR reminds me of this blog post: http://www.leancrew.com/all-this/2011/12/more-shell-less-egg/

Basically, we shouldn't use overcomplicated and overkill tooling and environments (Node in this case) when a simple shell script will do the same job.

@PeterJohnson
Copy link
Member

Consensus is not to merge this as-is. A less complex post-hook (e.g. shell script) might be more acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants