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: Lint and format only staged changes #197

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

AlexVascon
Copy link

@AlexVascon AlexVascon commented May 1, 2024

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

Screenshot 2024-05-07 at 16 21 09 Screenshot 2024-05-07 at 16 22 12

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.

Screenshot 2024-05-07 at 16 58 52

Add screenshots of relevant screens

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • [ x] I have followed the coding guidelines
  • [x ] My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • [ x] There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • [x ] I have made the necessary updates to the documentation, or no documentation changes are required.

Type

enhancement, bug_fix


Description

  • Integrated lint-staged in package.json to ensure only staged files are linted and formatted, improving performance.
  • Updated the Husky pre-commit hook to use lint-staged, streamlining the pre-commit checks.
  • Added lint-staged to devDependencies to manage the new dependency.

Changes walkthrough

Relevant files
Enhancement
package.json
Integrate lint-staged for Efficient Linting and Formatting

package.json

  • Added a new lint-staged configuration to handle linting and formatting
    of staged files.
  • Updated devDependencies to include lint-staged version ^15.2.2.
  • +10/-0   
    Configuration changes
    pre-commit
    Update Husky Pre-commit Hook to Use lint-staged                   

    .husky/pre-commit

  • Replaced direct lint and format commands with npx lint-staged to
    utilize the new lint-staged configuration.
  • Retained existing pnpm test:api command.
  • +1/-1     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    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
    @codiumai-pr-agent codiumai-pr-agent bot added type: enhancement New feature or request bug_fix labels May 1, 2024
    Copy link

    PR Description updated to latest commit (34ac617)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and involve configuration adjustments in package.json and the husky pre-commit hook. The logic is simple, and the impact is limited to linting and formatting operations.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: Ensure that the '--' in "pnpm run lint --" correctly prevents additional arguments from being passed to the turbo command as intended. This needs to be verified to ensure it behaves as expected in all scenarios.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add linting and formatting for CSS files to the lint-staged configuration.

    Consider adding a linting and formatting step for CSS files in the "lint-staged"
    configuration to maintain consistent code style across different file types.

    package.json [124-131]

     "lint-staged": {
       "*.{js,ts,tsx}": [
         "pnpm run lint --",
         "pnpm run format"
       ],
       "*.json": [
         "pnpm run format"
    +  ],
    +  "*.{css,scss}": [
    +    "pnpm run format"
       ]
     }
     
    Best practice
    Use pnpm instead of npx for running lint-staged in the pre-commit hook to maintain consistency.

    Replace npx lint-staged with pnpm lint-staged to ensure consistency with the package
    manager used throughout the project, which is pnpm.

    .husky/pre-commit [4]

    -npx lint-staged && pnpm test:api
    +pnpm lint-staged && pnpm test:api
     
    Maintainability
    Align the version of "lint-staged" with other dependencies' versions.

    Ensure that the version of "lint-staged" added to "devDependencies" matches the major
    version used in other dependencies to avoid potential conflicts and ensure compatibility.

    package.json [138]

    -"lint-staged": "^15.2.2",
    +"lint-staged": "^9.0.11",
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    npx lint-staged && pnpm test:api
    Copy link
    Member

    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.

    Copy link
    Contributor

    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

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented May 1, 2024

    @kriptonian1 Can you share your reviews on this?

    @rajdip-b rajdip-b changed the title fix: only lint and format staged changes chore: Lint and format only staged changes May 1, 2024
    @kriptonian1
    Copy link
    Contributor

    @AlexVascon Also do this a check https://www.npmjs.com/package/lint-staged#how-to-use-lint-staged-in-a-multi-package-monorepo
    since it's a monorepo, we should follow the format they mentioned in the doc

    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a 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}": [
    Copy link
    Contributor

    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

    Copy link
    Author

    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

    npx lint-staged && pnpm test:api
    Copy link
    Contributor

    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

    npx lint-staged && pnpm test:api
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented May 6, 2024

    hey bro @AlexVascon, could you please update us on this?

    @AlexVascon
    Copy link
    Author

    Hi sorry I do apologise. I was dealing with a medical issue. I will begin addressing your concerns right now

    @rajdip-b
    Copy link
    Member

    rajdip-b commented May 7, 2024

    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!

    @AlexVascon
    Copy link
    Author

    @AlexVascon Also do this a check https://www.npmjs.com/package/lint-staged#how-to-use-lint-staged-in-a-multi-package-monorepo since it's a monorepo, we should follow the format they mentioned in the doc

    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.
    @rajdip-b
    Copy link
    Member

    rajdip-b commented May 7, 2024

    Yes, your understanding is correct.

    Copy link

    sonarcloud bot commented May 7, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @AlexVascon
    Copy link
    Author

    Ok based on feedback I have made some changes.

    • I changed the pre-commit script to use pnpm instead of npx. That was an oversite on my part.
    • I also added all the file extensions I could find to add to the stage-lint script. Please let me know if you would like anything changed.
    • I could not add .json files for linting as you do not have any file extensions for it. You use various eslint-plugins but not eslint-plugin-json. Would you like me to add it? Otherwise I had to create a seperate script so only the formating is run for .json files
    • I also took some pictures which I will add to the PR description. These pictures represent the during and after the linting is run

    @rajdip-b
    Copy link
    Member

    rajdip-b commented May 9, 2024

    Hey @AlexVascon! Sorry for the late reply.

    Thanks for putting in all that effort!

    • No we won't need .json linting right yet because we aren't using any json files in particular
    • It would be real nice if you can upload those screenshots so that we know what to expect!
    • Also, can you resolve the merge conflict? Try pulling using git pull --rebase on your local branch. If the merge conflict still persists, delete the lockfile midway, use pnpm i then use git rebase --continue.

    @rajdip-b
    Copy link
    Member

    Hey @AlexVascon ! Any updates on this?

    @kriptonian1
    Copy link
    Contributor

    Hey @AlexVascon what's the upate

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type: enhancement New feature or request
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Lint files only in staging and not all files
    3 participants