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

chore: bump husky and clean up scripts #257

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

alcpereira
Copy link
Contributor

A few minor improvements that might be helpful for #253 :

  • Bumping Husky
  • Removing Husky from a child package (this will be handled by monorepo setup)
  • Some documentation on why .husky/install.mjs is present (almost removed it 😅 )
  • Simplifying .husky/pre-commit (see version 9.1.1)
  • Simplifying root package.json:
    • Both dependencies are actually dev dependencies
    • lint-staged script can be removed as it's directly ran by Husky now

@giovannibenussi
Copy link
Contributor

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?

@notrab
Copy link
Member

notrab commented Nov 1, 2024

@alcpereira @giovannibenussi should we update the .prettierrc file to contain the full configuration so it's more explicit for anyone not familiar with Prettier what's going on here?

@alcpereira
Copy link
Contributor Author

alcpereira commented Nov 1, 2024

@alcpereira @giovannibenussi should we update the .prettierrc file to contain the full configuration so it's more explicit for anyone not familiar with Prettier what's going on here?

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 --no-verify your commit and skip CI, but at this point we can't do anything for you).

@notrab
Copy link
Member

notrab commented Nov 1, 2024

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 prettier file that by glancing over the defaults, it resembles some formatting tooling.

Anyways, just my opinion. Continue as is by all means!

Copy link
Contributor

@giovannibenussi giovannibenussi left a 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!

@alcpereira
Copy link
Contributor Author

Can we merge this PR and I open another one to include a more verbose .prettierrc?

@giovannibenussi
Copy link
Contributor

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.

@penberg
Copy link
Contributor

penberg commented Nov 6, 2024

There's a merge conflict on package.json now that I merged bunch of other PRs.

@giovannibenussi
Copy link
Contributor

Fixed @penberg!

@penberg penberg merged commit 494c03c into tursodatabase:main Nov 6, 2024
6 checks passed
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