Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Have implementation for --no-* flags reflect documentation. #33

Closed
wants to merge 1 commit into from

Conversation

indexzero
Copy link

@indexzero indexzero commented Aug 2, 2018

The current documentation does not reflect the actual implementation. Quoting from that documentation

// flag with no value (-f, --force)
force: flags.boolean({
  char: 'f',
  // by default boolean flags may also be reversed with `--no-` (in this case: `--no-force`)
  // the flag will be set to false if reversed
  // set this to false to disable this functionality
  // allowNo: false,
}),

In cases such as this there can only be two outcomes: the docs are wrong, or the implementation is wrong. This PR assumes the implementation is wrong and updates the behavior to reflect the docs.

Remark: the documentation itself is somewhat ambiguous, but "set this to false to disable this functionality" implies that it is enabled by default.

@indexzero indexzero changed the title [fix test] Have implementation reflect documentation. Have implementation for --no-* flags reflect documentation. Aug 2, 2018
@codecov
Copy link

codecov bot commented Aug 2, 2018

Codecov Report

Merging #33 into master will increase coverage by 0.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   90.39%   90.72%   +0.33%     
==========================================
  Files          11       11              
  Lines         302      302              
  Branches       80       80              
==========================================
+ Hits          273      274       +1     
  Misses         10       10              
+ Partials       19       18       -1
Impacted Files Coverage Δ
src/flags.ts 100% <ø> (ø) ⬆️
src/parse.ts 91.97% <0%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2dac2d...4d37d92. Read the comment docs.

@jdx
Copy link
Contributor

jdx commented Aug 2, 2018

I feel we should change the docs actually. I prefer that implementation and this is a breaking change

@indexzero
Copy link
Author

@jdxcode I don't have strong feelings one way or the other. Requested documentation update in updated in oclif/oclif.github.io#13

@indexzero indexzero closed this Aug 2, 2018
@indexzero indexzero deleted the fix/no-bool branch August 3, 2018 09:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants