-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: auto-format using lint-staged and husky #71
Conversation
Looks great! My only concern with this type of setup is speed. The core repo freezes for at least a few seconds when committing, which is mildly annoying, but I assume that's either because of the size of the core codebase or the fact we're also tslinting and typechecking everything in the same step. |
@daun the problem with the setup we have in swup right now is that it checks the whole code base on each commit. That's exactly the selling point of lint-staged. Only check staged files, significantly speeding up the process 😎 |
@hirasso Sounds fantastic! |
@daun sorry I requested your review too early. Could you please read the "Questions" section in the PR description? I'm not completely sure how to proceed there – maybe a quick call would be helpful so that we can test the behavior together and then decide what route we want to take. |
@hirasso Sorry, I missed the question part entirely :) I'd opt for sticking with the built-in package script. Right now, prettier and the script produce the same output, but whenever we're changing that, we'll have one more thing to debug when these two start getting in each other's way. Then again, I don't care too much and won't lose sleep over it. |
@daun seems like reusing
Happy to walk you through the various options in a call. |
@hirasso Let's just use prettier directly via option 1 then — if it works, it works 🌝 |
7c9b941
to
44ba155
Compare
Description
Auto-format source and markdown files before committing changes to them. Uses lint-staged together with a husky
pre-commit
hook so that the formatting only runs on staged files before committing them, ignoring all other files (→ speed!!).If it turns out that this setup makes sense, we could think about adopting it in the other core plugins and swup itself. It would certainly make live easier for new contributors.
How to test...
...that unstaged files are being ignored
Expected Output in console:
...that staged files are being formatted
Expected output:
Questions
Right now I'm re-using the
swup package:format
script. That script currently ignores markdown files. I guess there would be no harm in adding them? Also, css (for fade-theme, for example).Alternative: Use prettier directly in lints-taged instead of the
swup: package:format
invocation:That's probably the more flexible solution anyways...
So lint-staged will even prevent empty commits if formatting the code undos all changes to the staged files 🎉
Checks
master
branchnpm run test
)