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

fix: don't die if manNumberRegex doesn't match the glob (#4610) #4611

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

d0sboots
Copy link
Contributor

@d0sboots d0sboots commented Mar 24, 2022

In certain edge cases, the glob could find files that manNumberRegex wouldn't match.

A possible solution would be to try to bring the two closer, but since globs and regexes are different syntaxes, the two will never be exactly the same.
It's always asking for bugs; better to just handle the issue directly.

In addition, when npm is installed globally in a system directory, the
glob can find other manpages with similar names.
For instance "install.1", "init.1", etc. Preferring low-numbered
sections isn't enough in these cases; it's also necessary to prefer
the pages prefixed with "npm-".

Fixes #4610

@d0sboots d0sboots requested a review from a team as a code owner March 24, 2022 09:14
lib/commands/help.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

This would need tests as well.

@nlf nlf added the pr: needs tests requires tests before merging label Mar 24, 2022
@d0sboots
Copy link
Contributor Author

I've added tests and significantly redid the PR. I also addressed an issue that I see as linked, but I can separate it out if you want - that frequently, npm help gets the wrong page, showing me system manpages instead of help for npm. This happens for me for npm help init, npm help install, and others.

@d0sboots
Copy link
Contributor Author

Amusingly, there's actually no check that npm help is getting help about npm at all. So npm help sudo works for me as an alternative to man sudo, for instance. But fixing that is out of scope for this...

lib/commands/help.js Outdated Show resolved Hide resolved
In certain edge cases, the glob could find files that manNumberRegex wouldn't match.

A possible solution would be to try to bring the two closer, but since globs and regexes are different syntaxes, the two will never be exactly the same.
It's always asking for bugs; better to just handle the issue directly.

In addition, when npm is installed globally in a system directory, the
glob can find other manpages with similar names.
For instance "install.1", "init.1", etc. Preferring low-numbered
sections isn't enough in these cases; it's also necessary to prefer
the pages prefixed with "npm-".
@d0sboots
Copy link
Contributor Author

Is there anything remaining here that I need to do?

@lukekarrys lukekarrys removed the pr: needs tests requires tests before merging label Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm help config fails with "Cannot read property '1' of null"
5 participants