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.

Sign up for GitHub

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

Jump to bottom

Prevent empty default value in CLI options #3441

Closed
wants to merge 1 commit into from
Closed

Prevent empty default value in CLI options #3441

wants to merge 1 commit into from

Conversation

outsideris
Copy link
Contributor

Description of the Change

If empty array is default value([]), it displayed as default:

With commander and a default value is an empty array([]), it shows like this:

--globals <names>                       allow the given comma-delimited global [names] (default: )

(default: ) is confusing to users.

So, this PR doesn't use a default value with an empty array. Instead, it set an empty array manually.

Alternate Designs

Let it as (default: ) or fix it in commander.js.

Possible Drawbacks

I'm not sure.

Applicable issues

Fix #3433

@outsideris
Prevent to empty default value in CLI options
e130e14 
If empty array is default value([]), it displayed as `default: `

Signed-off-by: Outsider <[email protected]>
@outsideris outsideris requested a review from markowsiak July 26, 2018 18:37
@outsideris outsideris added the area: usability concerning user experience or interface label Jul 26, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.289% when pulling e130e14 on outsideris:issue-3433 into 7941c9e on mochajs:master.

markowsiak
markowsiak approved these changes Aug 1, 2018
View reviewed changes
Copy link
Contributor

@markowsiak markowsiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@plroebuck
Copy link
Contributor

Certainly understand having fix on our side. But why was there no corresponding issue created for "tj/commander"? Would prefer to leave maintenance of their bugs to them.

@outsideris
Copy link
Contributor Author

@plroebuck As I said that in Alternate Designs, I'm not sure it's a bug in commander.js because it's related with how to use default vault and we can fix it on our side easily.
If we agree it is not our responsibility, I will report an issue or open the PR on commander.js.

@plroebuck
Copy link
Contributor

plroebuck commented Sep 2, 2018 •
edited
Loading

Think it's their problem - opened issue.
No problem with merging README updates in interim.

plroebuck
plroebuck reviewed Sep 2, 2018
View reviewed changes
docs/index.md
@@ -763,9 +763,9 @@ Mocha supports the `err.expected` and `err.actual` properties of any thrown `Ass
-w, --watch watch files for changes
--check-leaks check for global variable leaks
--full-trace display the full stack trace
--compilers <ext>:<module>,... use the given module(s) to compile files (default: )
--compilers <ext>:<module>,... use the given module(s) to compile files
Copy link
Contributor

@plroebuck plroebuck Sep 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for all of these README changes, why did you remove the default text altogether rather than just adding [] where it was missing?

plroebuck
plroebuck reviewed Sep 2, 2018
View reviewed changes
Copy link
Contributor

@plroebuck plroebuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we should hold off on "_mocha" changes until they respond to bug report.

@plroebuck
Copy link
Contributor

plroebuck commented Sep 2, 2018 •
edited
Loading

Tangentially related only.
Why are these default values hardcoded as strings?

Why not?

  .option('-s, --slow <ms>', '"slow" test threshold in milliseconds', Number, 75)
  .option('-t, --timeout <ms>', 'set test-case timeout in milliseconds', Number, 2000)

@outsideris
Copy link
Contributor Author

I see. I saw the opened PR in commander.js.
We can wait for a new release of commander.js.

@outsideris outsideris closed this Sep 3, 2018
@plroebuck
Copy link
Contributor

plroebuck commented Sep 3, 2018 •
edited
Loading

Uh, why'd you close this? The README is still messed up, and we'll still need to make changes to "package.json" and "package-lock.json" to fix even after their fix is available.

@plroebuck
Copy link
Contributor

plroebuck commented Sep 3, 2018 •
edited
Loading

Change title from:

Prevent to empty default value in CLI options

to

Prevent empty default value in CLI options.

@outsideris outsideris changed the title Prevent to empty default value in CLI options Prevent empty default value in CLI options Sep 5, 2018
@outsideris
Copy link
Contributor Author

@plroebuck As I leave comment in #3433 , we can open new PR after commander.js fixed it.
We can handle it in this PR. However, many opened PRs is not good to manage and new changes will be very different with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plroebuck plroebuck plroebuck left review comments

@markowsiak markowsiak markowsiak approved these changes

Assignees
No one assigned
Labels
area: usability concerning user experience or interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty default value in cli options
4 participants
@outsideris @coveralls @plroebuck @markowsiak