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

Document boolean flags by presence #283

Merged
merged 1 commit into from
Jul 27, 2019

Conversation

sixolet
Copy link
Contributor

@sixolet sixolet commented Jul 21, 2019

Add documentation that boolean flags should be specified by flag presence.

This is mutally exclusive with #284 .

Vote by thumbs up reaction emoji. Winner gets merged.

@sixolet sixolet added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 21, 2019
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 21, 2019
the flag *disables* a default behavior which is to do something, it should
start with `no` (eg. `--no-generate-name` when the default is to generate a
name).

Copy link
Contributor

@rhuss rhuss Jul 22, 2019

Choose a reason for hiding this comment

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

Actually, both variations (with and without --no) should be present in this case, regardless of the default value (using the variant that corresponds to the default value should be possible, too). In this example, e.g. --generate-name and --no-generate-name should be both available all the time (corresponding --generate-name=true and --generate-name=false).

The reason why I prefer this variant is:

  • There is no ambiguity what should be used as boolean value ("true", "yes" and "on" are commonly used as such values), where "true" is probably mostly accustomed for programmers (maybe not so much users of kn ?)
  • It's shorter without specifying the value
  • It stands in the tradition of GNU getopts (sometimes you see the variation --with- and --no- for true and false)
  • Many boolean options are modelled like this in Unix (they are called 'flags' in this case. Eg.: ls -l)
  • We already use that style e.g. for kn --help (or would we like to switch to kn --help=true ?). Other examples: kn service create --force or kn service create --async would have change to kn service create --force=true and kn service create --async=true (or maybe in the latter case --sync=false)
  • For consistencies reasons we would always have to require options with values then, which might lead to an unnecessarily bloated UI (because you can't use simple flags anymore).

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, both variations (with and without --no) should be present in this case, regardless of the default value (using the variant that corresponds to the default value should be possible, too).

I have the same feeling.

Choose a reason for hiding this comment

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

just noticed your statement here:

We already use that style e.g. for kn --help (or would we like to switch to kn --help=true ?). Other examples: kn service create --force or kn service create --async would have change to kn service create --force=true and kn service create --async=true (or maybe in the latter case --sync=false)

--help continues to work as cobra defaults it to mean true. so full form --flag=false is only necessary when turning things off.

Copy link
Contributor

@rhuss rhuss Jul 27, 2019

Choose a reason for hiding this comment

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

@cppforlife Have you tried kn --help=false ? My bad, it works as expected (no arg == help). You can use kn service list --help=false, too.

Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, sixolet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@duglin
Copy link
Contributor

duglin commented Jul 22, 2019

I think the most compelling argument made was:

It stands in the tradition of GNU getopts (sometimes you see the variation --with- and --no- for true and false)
Many boolean options are modelled like this in Unix (they are called 'flags' in this case)

The use of ..=true and ..=false always felt a bit odd to me.

Either PR can work though... my only really BIG sore point is if we allow --no-xxx=false. The double negative is just painful for people. So if we support ..=BOOL then we should never allow the name to start with no.

@sixolet sixolet removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2019
@knative-prow-robot knative-prow-robot merged commit 34f123c into knative:master Jul 27, 2019
maximilien pushed a commit to maximilien/client that referenced this pull request Jul 31, 2019
dsimansk added a commit to dsimansk/client that referenced this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants