fix(eslint-hook): ensure eslint hook receives arguments

[alveifbklsiu259]

fix(eslint-hook): ensure eslint hook receives arguments

SUMMARY

Issue
For some reason, the $@ variable is not correctly received by the eslint hook in pre-commit-config.yaml.

Solution
Moving the code to a separate file solves this issue.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

pre-commit-config.yaml

- repo: local
    hooks:
      - id: eslint
        name: eslint
        entry: bash -c 'echo "$@"; exit 1'
        language: system
        pass_filenames: true
        files: \.(js|jsx|ts|tsx)$

superset-frontend/src/components/Alert/index.tsx is modified, but the hook receives nothing.

This causes the eslint hook to always pass because no files are being passed to it (The actual issue is that $@ does not output all modified files, see comment).

Now, set entry back to bash -c 'cd superset-frontend && npm run eslint -- $(echo "$@" | sed "s|superset-frontend/||g")', and add this to the index:

Running eslint correctly catches the error.

But running eslint hook does not.

Moving the code to a separate file does solve the issue, it correctly receives the variable $@.
scripts/eslint.sh

echo "$@"
exit 1

pre-commit-config.yaml

- repo: local
    hooks:
      - id: eslint
        name: eslint
        entry: ./scripts/eslint.sh
        language: script
        pass_filenames: true
        files: \.(js|jsx|ts|tsx)$

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[korbit-ai]

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Missing error context for critical operations ▹ view	✅
	Missing directory existence check ▹ view	✅

Files scanned

​

File Path	Reviewed
scripts/eslint.sh	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

• You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
• You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
• Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
• Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
• Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

• Check out our docs on how you can make Korbit work best for you and your team.
• Customize Korbit for your organization through the Korbit Console.

Feedback and Support

• Tell us what you think of Korbit
• Schedule a call with our team
• Email us @ [email protected]

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.45%. Comparing base (76d897e) to head (8b71801).
Report is 1466 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #32333       +/-   ##
===========================================
+ Coverage   60.48%   83.45%   +22.96%     
===========================================
  Files        1931      546     -1385     
  Lines       76236    39097    -37139     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32629    -13485     
+ Misses      28017     6468    -21549     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.49% <ø> (-0.67%)	⬇️
javascript	?
mysql	75.85% <ø> (?)
postgres	75.92% <ø> (?)
presto	53.02% <ø> (-0.78%)	⬇️
python	83.45% <ø> (+19.96%)	⬆️
sqlite	75.43% <ø> (?)
unit	61.06% <ø> (+3.44%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rusackas]

Do we need the extra script file here, or does simply removing the sed business and letting the pre-commit hook run npm run lint (or even npm run lint-fix) from the pre-commit config do the job? I'm just hoping to reduce complexity/maintenance if it's not needed.

Also, regarding the type checking from the prior PR, I noticed that running pre-commit run --all-files is super slow and resource intensive. npm run type is MUCH faster. Do you think there's anything that can be improved on that front?

Appreciating the help here, by the way! :)

[mistercrunch]

Oh weird, I'm pretty sure the $@ trick work on my machine, is this an OS-specific issue (like it doesn't work as expected on Windows/Linux)?

In my experience eslint runs only on the files touched by the current commit (?)

[alveifbklsiu259]

Do we need the extra script file here, or does simply removing the sed business and letting the pre-commit hook run npm run lint (or even npm run lint-fix) from the pre-commit config do the job? I'm just hoping to reduce complexity/maintenance if it's not needed.

I created a separate file because $@ outputs nothing(see below) when used in pre-commit-config.yaml, even if there are touched files under superset-frontend in the index. However, $@ correctly outputs touched files when used in a separate script.

Oh weird, I'm pretty sure the $@ trick work on my machine, is this an OS-specific issue (like it doesn't work as expected on Windows/Linux)?

I'm not sure if this is a OS-specific issue, I'm running on WSL2 with Ubuntu 22.04.5.

---

After a thorough test, I found that when $@ is used in pre-commit-config.yaml, it does outputs touched files, but not all of them.

pre-commit-config.yaml

- repo: local
    hooks:
      - id: eslint
        name: eslint
        entry: bash -c 'echo "$@"; exit 1'
        language: system
        pass_filenames: true
        files: \.(js|jsx|ts|tsx)$

superset-frontend/src/components/Avatar/index.tsx is touched, but $@ outputs nothing. (This is the reason why I initially thought $@ outputs nothing)

When more files are added to the index, $@ starts to output touched files, but some files are missing.

superset-frontend/src/components/Avatar/index.tsx is missing from $@.

superset-frontend/src/components/Avatar/index.tsx and superset-frontend/src/components/Alert/index.tsx are missing from $@.

But if we use $@ in a separate files, it seems to solve this issue.

scripts/eslint.sh

#!/usr/bin/env bash

echo "$@"; exit 1

pre-commit-config.yaml

- repo: local
    hooks:
      - id: eslint
        name: eslint
        entry: ./scripts/eslint.sh
        language: script
        pass_filenames: true
        files: \.(js|jsx|ts|tsx)$

All touched files are included.

[alveifbklsiu259]

@rusackas

Also, regarding the type checking from the prior PR, I noticed that running pre-commit run --all-files is super slow and resource intensive. npm run type is MUCH faster. Do you think there's anything that can be improved on that front?

The resource-intensive issue of running pre-commit type-checking-frontend --all-files should be fixed after the merge of 32323.

Simply put, by default each pre-commit hook runs in parallel with a different set of files. This means that when you run --all-files, the hook type-checking-frontend runs the corresponding script 16 times(according to my test), causing tsc to run 16 times. This is solved by running the hook sequentially.

As for the execution time compared to npm run type, running pre-commit type-checking-frontend --all-files takes almost twice the amount of time because of command line length limits, it's not feasible to pass all files to a single command invocation. Instead, pre-commit splits the files into manageable chunks and processes them. (2 chunks when running sequentially according to my test)

A more detailed explanation can be found in 32323.