-
-
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
update docs with globbing and shell expansion details; closes #3136 #3348
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.
Thanks for this! I have a suggestion.
Also, these changes failed the Markdown lint. run npm start lint.markdown
to check the Markdown locally.
docs/index.md
Outdated
$ mocha ./spec/*.js | ||
``` | ||
|
||
If you use shell expansion for this, you must wrap your glob pattern in double quotes (note, using the `--recursive` flag here is useless): |
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.
A lot of people probably doesn't know what "shell expansion" is.
In fact, I would probably remove L1279-1283 and just give the example below with double quotes. Then, explain below the example how using **
means --recursive
is useless. Recommend using double quotes for "portability"; otherwise it'll need a pretty long-winded explanation.
@boneskull I made the updates you recommended, except for "using The lines above my changes read:
This is true by default in Bash, but the I believe ZSH and Fish support recursive matching for edit: added example |
docs/index.md
Outdated
|
||
```sh | ||
$ mocha "./spec/**/*.js" | ||
``` |
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 didn't know about the globstar in Bash 4.3.
I feel that current explanation is a little bit confused. If we explain the globstar explicitly, users can understand clearer because the behavior of **
is depend on a shell.
@boneskull @outsideris I've updated the changes to reflect your feedback, please give it another look when you can! |
hmmm.. Why did builds failed for this PR? |
Aha! The failure is related with this. |
@outsideris @boneskull I rebased from develop with the CI fixes, let me know if you need me to adjust anything else |
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
4547268
to
7613521
Compare
@boneskull do you have more feedback on this? |
@boneskull @outsideris let's get this merged! (please 😄 ) |
Thank you. |
$ mocha "./spec/**/*.js" | ||
``` | ||
|
||
*Note*: Double quotes around the glob are recommended for portability. |
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.
Disagree with this statement.
If you want portability, then single quotes are required; using double quotes allows possibility of shell expansion, etc. Using single quotes tells the shell "hands off", and Mocha's lookupFiles
would handle the glob expansion.
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.
@plroebuck thanks for the feedback. My understanding is that glob expansion would only happen when using parameter expansion inside double quotes, see https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion.
FWIW the context for specifying double quotes is here #3136 (comment).
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.
Yes, you're correct that double quotes are required if you want the shell to do the job. Perhaps I misunderstood the intent of this update, assuming it was to explain how to target files recursively (via glob) in general. While knowing how to do this via UNIX shell is useful (but unnecessary here), the portable recommendation would be to use single quotes, sidestepping platform differences and characteristics of various shells with various options disabled/enabled.
To configure where `mocha` looks for tests, you may pass your own glob: | ||
|
||
```sh | ||
$ mocha --recursive "./spec/*.js" |
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.
In this example, the glob is unnecessary.
$ mocha --recursive spec
…#3136 (mochajs#3348) * update docs with globbing and shell expansion details; closes mochajs#3136 * requested changes * add explanation for recursive matching for different shells
Add documentation around shell expansion and globbing for passing files to mocha