-
-
Notifications
You must be signed in to change notification settings - Fork 93
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: Lint and format only staged changes #197
base: develop
Are you sure you want to change the base?
chore: Lint and format only staged changes #197
Conversation
This commit adds the package lint-staged. I think when lint-staged is calling pnpm run lint it passes some file arguments. Since pnpm run lint calls turbo run lint this argument is getting passed directly resulting in an error as extra arguments get added to the tubro command. to prevent this we add '--' to signigy not to pass on further arguments. :wq :wq :wq wq qw :qw
PR Description updated to latest commit (34ac617) |
PR Review
✨ Review tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
See the improve usage page for a comprehensive guide on using this tool. |
.husky/pre-commit
Outdated
npx lint-staged && pnpm test:api |
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.
Since we are using pnpm, i believe pnpx
would be a better thing to use in here.
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.
Why have we removed pnpm format from here
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.
We can use task concurrency ig @rajdip-b
https://www.npmjs.com/package/lint-staged#task-concurrency
@kriptonian1 Can you share your reviews on this? |
@AlexVascon Also do this a check https://www.npmjs.com/package/lint-staged#how-to-use-lint-staged-in-a-multi-package-monorepo |
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 believe that once this review has been resolved, we will be prepared to proceed.
package.json
Outdated
@@ -121,11 +121,21 @@ | |||
"prepare": "husky install", | |||
"sourcemaps:api": "turbo run sourcemaps --filter=api" | |||
}, | |||
"lint-staged": { | |||
"*.{js,ts,tsx}": [ |
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.
This will be only ts, and tsx, and now js
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.
This is true. I have added more extensions that I found your project uses. Let me know if you would like any further changes. I could not add .json files for the linting as you do not have any configuration for it. You use various eslint-plugins but not eslint-plugin-json. Would you like me to add it? otherwise I can only run the formatter which is why I added a seperate script for it in the lint-stage
.husky/pre-commit
Outdated
npx lint-staged && pnpm test:api |
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.
Why have we removed pnpm format from here
.husky/pre-commit
Outdated
npx lint-staged && pnpm test:api |
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.
We can use task concurrency ig @rajdip-b
https://www.npmjs.com/package/lint-staged#task-concurrency
hey bro @AlexVascon, could you please update us on this? |
Hi sorry I do apologise. I was dealing with a medical issue. I will begin addressing your concerns right now |
Hey man. I hope you are okay! Incase you need more time, please feel free to. Health is important! |
Please correct me if I am wrong but my understanding is you guys are using turbo which acts as a kind of central hub to execute commands across all packages in the monorepo. By defining linting at the root level and invoking it using Turbo mode (turbo run lint), we have centralized the linting configuration for your entire monorepo. This means that when you run linting commands from the root, they apply to all packages in the monorepo. (I had some help from chatGPT to help me understand) This makes sense since initially in the pre-commit file you ran pnpm lint which in the root package.json runs the script "lint": "turbo run lint". |
…e extension .json files could not be added for the linting because none of your linters are configured for it. You use various eslint-plugins but don't use eslint-plugin-json. Do you want this added? Otherwise we can only run formatting for json files which is why a seperate command is in place for that in the lint-staged script.
Yes, your understanding is correct. |
Quality Gate passedIssues Measures |
Ok based on feedback I have made some changes.
|
Hey @AlexVascon! Sorry for the late reply. Thanks for putting in all that effort!
|
Hey @AlexVascon ! Any updates on this? |
Hey @AlexVascon what's the upate |
@AlexVascon are you still working on this issue |
User description
Description
This commit adds the package lint-staged. This package makes sure only staged changes are scanned for linting and formating instead of every file. I think when lint-staged is calling pnpm run lint it passes some file arguments. Since pnpm run lint calls turbo run lint this argument is getting passed directly resulting in an error as extra arguments get added to the tubro command. to prevent this we add '--' to signigy not to pass on further arguments.
I based this from a comment made by github user mehulkar whos explanation and solution helped me solve this problem.
link here
Give a summary of the change that you have made
Changes are made to 2 files.
package.json root
.husky/pre-commit
In the package.json a section is included for lint-staged. Within 2 commands are set so only lint and formating are run on staged commits and not the whole file. These commands where moved from the husky pre-commit file and replaced with pnpm run lint-staged. This calls the code under lint-staged in the package.json.
Fixes #195 []
issue 195
link here
This commit fixes the issue where all files got scanned for linting and formating which is very slow and it's better that only the staged files get scanned as they contain changes.
Dependencies
lint-staged
Mention any dependencies/packages used
lint-staged
Future Improvements
Mention any improvements to be done in future related to any file/feature
Mentions
Mention and tag the people
Screenshots of relevant screens
This screenshot below is what it looks like if you dont use lint-staged. I edited the pre-commit file and changed pnpm lint-staged to pnpm lint.
Add screenshots of relevant screens
Developer's checklist
If changes are made in the code:
Documentation Update
Type
enhancement, bug_fix
Description
lint-staged
inpackage.json
to ensure only staged files are linted and formatted, improving performance.lint-staged
, streamlining the pre-commit checks.lint-staged
todevDependencies
to manage the new dependency.Changes walkthrough
package.json
Integrate lint-staged for Efficient Linting and Formatting
package.json
lint-staged
configuration to handle linting and formattingof staged files.
devDependencies
to includelint-staged
version^15.2.2
.pre-commit
Update Husky Pre-commit Hook to Use lint-staged
.husky/pre-commit
npx lint-staged
toutilize the new lint-staged configuration.
pnpm test:api
command.