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

♻️ Replace ESLint and Prettier with Biome to format and lint frontend #719

Merged
merged 6 commits into from
Mar 17, 2024

Conversation

santigandolfo
Copy link
Contributor

I had to modify the modify-openapi-operationids.js script because biome was giving me a lint error because of the use of forEach instead of for...of

I also disabled some other rules in the linter that were making the project fail without some changes, because I'd prefer the maintainers decide if we want to keep it as it is or not.

@santigandolfo
Copy link
Contributor Author

I see that this other PR has been merged: #714
I see two alternatives:

  1. Modify my PR to instead use biome in pre-commit: https://biomejs.dev/recipes/git-hooks/#pre-commit
  2. Revert the changes made in ⚙️ Update pre-commit config with Prettier and ESLint #714 and keep just my changes

The only reason why I prefer 2 over 1 is that using that approach the git-hooks on the frontend are installed automatically when we run npm install, while pre-commit hooks are installed only if we run pre-commit install.
I think that we should consider which trade off we want to have, either have different ways we define linting git-hooks for the frontend and backend or we have a standarized way but are forced to run pre-commit install everytime we want to setup the project (currently we never mention in the docks that the pre-commit hooks should be installed)

@tiangolo
Copy link
Member

Awesome! This looks cool @santigandolfo! 🚀

We need to keep pre-commit because that's what's used to format the backend and the rest of the code, and Husky expects to be the only git hook handler, disabling pre-commit. 🤓

I'll ask @alejsdev to tweak this PR a bit with some commits on top to adjust the rules to the current code style, to minimize the changes, but taking in a few of them, etc. And then it will be ready, thank you! 🍰

@tiangolo tiangolo changed the title Replaced eslint and prettier with biome ♻ Replace ESLint and Prettier with Biome to format and lint frontend Mar 17, 2024
@alejsdev alejsdev changed the title ♻ Replace ESLint and Prettier with Biome to format and lint frontend ♻️ Replace ESLint and Prettier with Biome to format and lint frontend Mar 17, 2024
@tiangolo tiangolo merged commit 43435d5 into fastapi:master Mar 17, 2024
3 checks passed
@tiangolo
Copy link
Member

Awesome, thanks @santigandolfo and @alejsdev! 🍰

gusevyaroslove pushed a commit to gusevyaroslove/fastapi-template that referenced this pull request Aug 4, 2024
…fastapi#719)

Co-authored-by: User <alejsdev@gmail.com>
Co-authored-by: Alejandra <90076947+alejsdev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants