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(count): do not increment a default value #39

Merged
merged 1 commit into from
Jul 16, 2016
Merged

Conversation

nexdrew
Copy link
Member

@nexdrew nexdrew commented Jul 15, 2016

Based on yargs/yargs#500

This was a bit tricky to figure out because default values were already being applied in line 274 but were then being overwritten immediately thereafter in a weird attempt to use the increment function to set a default value of 0 when no explicit default value is given. When an explicit default value was given, the increment function would then inadvertently add/concat a 1 to the value. 😕

Therefore the fix is comprised of these changes:

  1. Let applyDefaultsAndAliases do its job of setting explicit defaults, see line 274
  2. Apply a default of 0 to a count only when (a) no arg is given and (b) no explicit default is set, see new line 278
  3. Only use the increment function when a count arg is actually given (not when setting a default value), see new line 339
  4. When setting a default value for a count, use the value as given (don't use increment and don't wrap it in an array), see new line 519
  5. Since increment is now only used for when an arg is given, use a default of 1 instead of 0, see new line 674

Through testing, I also found that previous functionality treated all arg values for a count flag equally (they all increment the count, regardless of value). This has been preserved, codified in a new test. We can consider changing this later if we want.

Hopefully this makes the logic a little more straightforward. 😎

@coveralls
Copy link

coveralls commented Jul 15, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ff1c627 on fix-count-with-default into 2418bbc on master.

@@ -1263,6 +1263,60 @@ describe('yargs-parser', function () {
parsed.verbose.should.equal(2)
parsed.should.have.property('_').and.deep.equal(['moomoo'])
})

it('should use a default value as is when no arg given', function () {
Copy link
Member

Choose a reason for hiding this comment

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

lovely, glad to have these additional tests for count.

@bcoe bcoe merged commit b04a189 into master Jul 16, 2016
@bcoe bcoe deleted the fix-count-with-default branch July 16, 2016 22:47
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.

3 participants