-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
docs: fix documentation concerning glob expansion on UNIX #4869
Conversation
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 agree the docs is incorrect, but I'm not happy with your text either. I'm unsure about your Windows statement since we have countless shells in Windows as eg. git bash, powershell, wsl, cmd, etc. Especially powershell is a pain being so different compared to posix-like shells.
So what the docs should tell (in your own words):
- use quotes to prevent glob expansion by the shell, single/double quotes depending on shell
- it has to be Mocha (via its dependency
node-glob
) which does the glob expansion - we recommend double quotes for portability.
Every additional info may be confusing or incorrect, I prefer to not document on shell detail level.
If you shellout in Windows, which is what |
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.
LGTM, thanks! 🙌
Just one suggestion around a friendlier docs link. What do you think?
Co-authored-by: Josh Goldberg ✨ <[email protected]>
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 would simplify it to:
[You should _always_ quote your globs in npm scripts][article-globbing], the [`node-glob`][npm-glob] module will handle its expansion.
Lets keep additional specifics of shells and os:es out of it.
mochajs#4869 (review) requested that platforms and shells not be mentioned. This means that rationale for the directions cannot be provided. The article which was originally linked suggests to use single quotes which do not work on Windows. For example, using single quotes for a glob expression referring to a folder or file with `&` in its name would break the expression into two commands on Windows. Since the rationale was removed and the linked article was misleading, instructions on how to correctly quote globs were added. The rationale is recorded here since it isn’t allowed in the instructions: * Since a Unix-like OS user would be accustomed to referencing a path containing `"` in it by writing `\"` within a double-quoted string, I mentioned that the double quote and backslash characters are forbidden. * Since a Windows user would be accustomed to including `$` in file names but double quoted strings have parameter expansion performed on them on on Unix-like OSes, I had to mention that `$` is a forbidden character. * Since Windows doesn’t support single quotes, I had to mention that double quotes are required.
I addressed your changes in the latest commit. I checked out the article you are referring to and found that it is misleading. It suggests to use single quotes which are unsupported on Windows. If a folder name contains an ampersand (legal on Windows and Unix-like OSes), then the script author might create an expression which succeeds on a Unix-like OS and fails on Windows. So I removed that reference too. Since the rationale cannot be included, I needed to add instructions to:
Please consider the latest changes. |
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.
Better! Thanks 🙏 And thanks for removing the erroneous article.
How would you feel about this minor wording tweak?
Co-authored-by: Pelle Wessman <[email protected]>
Thanks @binki 🙏 |
Fixes #4868.
Requirements
Description of the Change
Fix documentation about how shell expansion works on UNIX.
Alternate Designs
I removed incorrect information and added the equivalent correct information. I may have been more verbose than necessary, but it seemed like the documentation was trying to be informative here, so I continued providing the supplemental information.
Why should this be in core?
This is a documentation fix. It removes erroneous information in core which should not be there.
Benefits
The documentation will be less wrong.
Possible Drawbacks
This may result in people being less confused.
Applicable issues
#4868