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: render default arg/opt if equal to 0 #264

Merged
merged 2 commits into from
Aug 11, 2021
Merged

Conversation

akwodkiewicz
Copy link

This PR fixes rendering of default values for arguments and flags.
Before the fix, if an arg/flag default was set to 0, it was not rendered.

I couldn't reopen or edit my existing PR, so this is an updated version of #248, but the base branch is set to main this time.

Copy link
Contributor

@RodEsp RodEsp left a comment

Choose a reason for hiding this comment

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

I'd rather see us explicitly call out equality to 0 to avoid other unintended values.

src/command.ts Outdated Show resolved Hide resolved
src/command.ts Outdated Show resolved Hide resolved
@akwodkiewicz
Copy link
Author

akwodkiewicz commented Aug 6, 2021

I deliberately used my approach, because the default field is (incorrectly1) typed as a string, so the compiler does not allow me to check for flag.default === 0, because it believes it's a (constant) false expression. My approach allowed me to leave the types unchanged (I did not want to cast fields to other types nor change the typings in other repositories) and still fix the bug.

I took a look at the oclif/parser and I believe this 2 is probably why the type of a default is not a string, but anything that is assigned to the default parameter.

Footnotes

  1. If the value was in fact a string, then Boolean('0') === true and the zero would be printed. And I wouldn't need to write this PR 😄

  2. https://github.com/oclif/parser/blob/master/src/parse.ts#L214

@RodEsp
Copy link
Contributor

RodEsp commented Aug 9, 2021

@akwodkiewicz good point. I updated my original suggestions a little bit to account for that.

@akwodkiewicz
Copy link
Author

@RodEsp, thanks for the changes. Should I add an inline comment explaining why we're calling .toString() on a string or will a commit message be sufficient?

@RodEsp
Copy link
Contributor

RodEsp commented Aug 10, 2021

@akwodkiewicz a comment would be great 🙂

When the default value for an arg/flag was set to `0` then the `--help`
was not displaying it, as if the default was not set.

There is probably a typing bug in oclif/config or oclif/parser
(depending on what's the desired outcome). The default values typing
is `string`, however the actual values can be set to numbers.
@akwodkiewicz
Copy link
Author

@RodEsp , I have made the changes

src/command.ts Outdated Show resolved Hide resolved
src/command.ts Outdated Show resolved Hide resolved
@RodEsp RodEsp merged commit ca0af19 into oclif:master Aug 11, 2021
@RodEsp
Copy link
Contributor

RodEsp commented Aug 11, 2021

Thanks for the contribution @akwodkiewicz!

ghost pushed a commit that referenced this pull request Aug 11, 2021
## [3.2.3](v3.2.2...v3.2.3) (2021-08-11)

### Bug Fixes

* render default arg/opt if equal to 0 ([#264](#264)) ([ca0af19](ca0af19))
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.

None yet

2 participants