mocha icon indicating copy to clipboard operation
mocha copied to clipboard

Fix documentation concerning glob expansion on UNIX.

Open binki opened this issue 2 years ago • 1 comments

Fixes #4868.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

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

binki avatar Apr 11 '22 03:04 binki

we have countless shells in Windows as eg. git bash, powershell, wsl, cmd, etc.

If you shellout in Windows, which is what npm run does, you get CMD. I do not think that will change in the foreseeable future. People who use different shells on Windows are either power users and know what they are doing or are being told to do so by other people who they can ask for help.

binki avatar Apr 15 '22 13:04 binki

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: binki / name: Nathan Phillip Brink (ead1635e782aec843ac69f70897c29176da40515, 7129fbfa186443838e50ab8d6063ec09f8c73b3a, ab6bed3b4d8aaa32669aa4f314cbd5c63cb0d059, d5c86e23084af8c0f826ccc093f60867868a4f89)

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.

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:

  • use double quotes
  • avoid obvious problem-causing characters within the double quotes

Please consider the latest changes.

binki avatar Mar 12 '24 16:03 binki

Thanks @binki 🙏

voxpelli avatar Mar 12 '24 17:03 voxpelli