-
Notifications
You must be signed in to change notification settings - Fork 39
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
chore: bump husky and clean up scripts #257
Conversation
Great work on this one @alcpereira! I haven’t tested this, but does it fix the formatting issues if you try to push an unformatted file? |
@alcpereira @giovannibenussi should we update the |
As you prefer, IMO most JS developers are used to Prettier default configuration -it has been the standard for years- and it's common practice to just include what should be overwritten in the config. Including all defaults, just to be explicit, can be a bit cumbersome due to the quantity of options (docs). Also by including formatting in the pre-commit hook + format check in CI, this issue should never happen again (well you can |
I do agree that Prettier is a common standard. I've used Prettier since day one, and it's great. But even by state of JS survey standards, there's still a chunk of folks not heard of it or use it regularly, including some maintainers of this repo. It's good we have CI now, so people don't need to worry about it. My thought for including the default options (as gross as that is btw) would make it more clear to those discovering a Anyways, just my opinion. Continue as is by all means! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote to keep the config file as it is too.
Changes look good to me!
Can we merge this PR and I open another one to include a more verbose |
I think the current file is fine. I think it's more confusing for devs to have a setting for something that must be the default (it makes me wonder why it's explicitly defined) and also adds more noise to the whole file. There's a point to be more verbose but I think it's better to leave it as it is. |
There's a merge conflict on |
Add missing comma to package.json
Fixed @penberg! |
A few minor improvements that might be helpful for #253 :
.husky/install.mjs
is present (almost removed it 😅 ).husky/pre-commit
(see version 9.1.1)package.json
:lint-staged
script can be removed as it's directly ran by Husky now