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

feat: allow a path to be supplied to the --shell option (#993) #994

Merged
merged 2 commits into from Jul 22, 2021
Merged

Conversation

@iiroj
Copy link
Collaborator

@iiroj iiroj commented Jul 20, 2021

See issue #993 for info.

@iiroj iiroj linked an issue that may be closed by this pull request Jul 20, 2021
@codecov
Copy link

@codecov codecov bot commented Jul 20, 2021

Codecov Report

Merging #994 (8620c82) into master (7734156) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #994   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        19    +1     
  Lines          612       633   +21     
  Branches       145       148    +3     
=========================================
+ Hits           612       633   +21     
Impacted Files Coverage Δ
lib/index.js 100.00% <100.00%> (ø)
lib/messages.js 100.00% <100.00%> (ø)
lib/symbols.js 100.00% <100.00%> (ø)
lib/validateOptions.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7734156...8620c82. Read the comment docs.

Loading

@iiroj iiroj requested a review from okonet Jul 20, 2021
@iiroj
Copy link
Collaborator Author

@iiroj iiroj commented Jul 22, 2021

What do you think, @okonet? I guess we could improve the error message for when the shell is incorrect, since now it just prints an ENOENT. Then again, I think this is an advanced feature and this change should be kept as simple as possible.

❯ node bin/lint-staged.js --debug --shell "not-found-shell"
Running lint-staged with the following config:
{
  '*.js': 'eslint --fix',
  '*.{json,md}': 'prettier --write'
}
[STARTED] Preparing...
[SUCCESS] Preparing...
[STARTED] Running tasks...
[STARTED] Running tasks for *.js
[STARTED] Running tasks for *.{json,md}
[SKIPPED] No staged files match *.{json,md}
[STARTED] eslint --fix
[FAILED] eslint --fix [ENOENT]
[FAILED] eslint --fix [ENOENT]
[SUCCESS] Running tasks...
[STARTED] Applying modifications...
[SKIPPED] Skipped because of errors from tasks.
[STARTED] Reverting to original state because of errors...
[SUCCESS] Reverting to original state because of errors...
[STARTED] Cleaning up...
[SUCCESS] Cleaning up...

✖ eslint --fix failed without output (ENOENT).

Loading

@okonet
Copy link
Owner

@okonet okonet commented Jul 22, 2021

I think it’s fine unless it’s a simple change to add this information.

Loading

@iiroj
Copy link
Collaborator Author

@iiroj iiroj commented Jul 22, 2021

I can try creating a simple fs.promises.access check with the fs.constants.X_OK mode. It's just that in order to do it properly, we should also have a validateOptions method in addition to the validateConfig. Oh well, it's a good refactor in any case.

Loading

@iiroj
Copy link
Collaborator Author

@iiroj iiroj commented Jul 22, 2021

Yeah, it was doable:

❯ node bin/lint-staged.js --shell "not-found-shell"
✖ Validation Error:

  Invalid value for option shell: not-found-shell

  ENOENT: no such file or directory, access 'not-found-shell'

See https://github.com/okonet/lint-staged#command-line-flags

Loading

@iiroj
Copy link
Collaborator Author

@iiroj iiroj commented Jul 22, 2021

What do you think @okonet and @Kurt-von-Laven?

Btw @okonet you need to approve the PR before I can merge it.

Loading

okonet
okonet approved these changes Jul 22, 2021
@iiroj iiroj merged commit fea8033 into master Jul 22, 2021
11 checks passed
Loading
@iiroj iiroj deleted the shell-string branch Jul 22, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Jul 22, 2021

🎉 This PR is included in version 11.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Loading

@Kurt-von-Laven
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven commented Jul 23, 2021

Just tested this out, and encountered the following error:

✖ Validation Error:

  Invalid value for option shell: --relative

  ENOENT: no such file or directory, access '--relative'

See https://github.com/okonet/lint-staged#command-line-flags
husky - pre-commit hook exited with code 1 (error)

If I swap the order of the arguments, I instead get:

error: option '-x, --shell <path>' argument missing
husky - pre-commit hook exited with code 1 (error)

It appears that support for the case where no shell is specified was dropped in this PR, making this a breaking change rather than a simple feature release. How much work would it be to add back support for a boolean --shell?

Loading

@iiroj
Copy link
Collaborator Author

@iiroj iiroj commented Jul 24, 2021

@Kurt-von-Laven thanks for detecting this, it seems the tests did not catch it! I'll see how it can be fixed.

EDIT:

It was a simple fix. Commander treats <path> as required and [path] as optional, so using the latter helps:

❯ node bin/lint-staged.js --shell --relative
options { shell: true }
ℹ No staged files found.

❯ node bin/lint-staged.js --shell 'test' --relative
options { shell: 'test' }
ℹ No staged files found.

Loading

iiroj added a commit that referenced this issue Jul 24, 2021
This fixes a bug introduced in #994 where any text after the `--shell` option was interpreted as its value, for example in `--shell  --relative` the value was `"--relative"` instead of implied `true`.
iiroj added a commit that referenced this issue Jul 24, 2021
)

This fixes a bug introduced in #994 where any text after the `--shell` option was interpreted as its value, for example in `--shell  --relative` the value was `"--relative"` instead of implied `true`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants