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

Add support for --no-option #334

Closed
TheConstructor opened this issue Jan 16, 2020 · 18 comments
Closed

Add support for --no-option #334

TheConstructor opened this issue Jan 16, 2020 · 18 comments
Labels
closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. enhancement stale

Comments

@TheConstructor
Copy link
Contributor

I got two ideas on how to do this:

  1. This could automatically be applied to bool?-type options making all three states reachable
  2. This could be enabled by specifying a no-value. This would enable ./configure like --no-dep/--dep=xyz stuff

I don't have an idea how to apply this to short-options (--no-s as negation of -s feels slightly off).

Currently you can define a second option to get a similar behavior. Main benefit would be less boilerplate and a shorter help-text.

I am willing to implement this myself if the idea sounds sound to you.

@natemcmaster
Copy link
Owner

I've seen the --[no-]option pattern before, but struggling to remember where and how it was useful. Do you have some concrete examples of what a user would do with this?

@natemcmaster
Copy link
Owner

After though: I've also seen --[disable/enable]-feature, so need to be careful about being too opinionated on naming conventions.

@TheConstructor
Copy link
Contributor Author

We have some options wich csn be enabled in configuration. Now there might be an occasional call where you want to temporarily override configuration. Also if you are writing scripts and don't control the local configuration, you may want to explicitly state, that is is off. (Verbose, color-output, …)

You can do this with an (optional) value, but I prefer --no-force over --force false as it will not mess with arguments following it, and at the same time you have --force and not --force true. Lastly I would like to link enable-/disable-switches in help output.

While I may prefer --no-… adding a way to template/specify --disable-… should be straight forward.

@natemcmaster
Copy link
Owner

I think this idea needs to be fleshed out a little more. I thought I knew what you were asking, but then your response didn't fit with my original assumptions. Maybe I'm just dense. Can you share some code snippets of what you think the completed feature would look like for the end user?

@JakeSays
Copy link

JakeSays commented Feb 13, 2020

@natemcmaster this is a common pattern used in compilers and other such tools (configure, etc) and is used to invert the default meaning of a switch.

However I do believe @TheConstructor's definition is a little too broad and confusing. I suggest this feature be limited to boolean, long named switches. The absence of a valued option indicates --no-dep.

Controlling compiler warnings:

-Wall
-Wno-unused-parameter
-Wno-whatever

The first switch enables all warnings, then the following two disable specific warnings, and the opposite is also true:

-Wunused-parameter
-Wwhatever

By default all warnings are disabled, and we're enabling two of them.

I propose the following syntax:

(pseudo code, and naming conventions are just suggestions)

enum ToggleState
{
    Unchanged,
    No,
    Yes
}

var option1 = app.Option("--no-switch", CommandOptionType.Toggle);
var option2 = app.Option("--disable-other-switch", CommandOptionType.Toggle);

if (option1.ToggleValue == ToggleState.Yes)
{
    //do whatever makes sense
}

Usage is myapp --switch --other-switch or myapp -no-switch -disable-other-switch, and the meaning of the presence or absence of the prefix is determined by the developer.

The toggle prefix is declared by convention as the first stanza in the option declaration string. Presence of the prefixed switch indicates ToggleState.Yes, unprefixed switch ToggleState.No, and no switch ToggleState.Unchanged.

An alternative prefix specification is --enable|disable-switch to enable both --enable-switch and --disable-switch, which is also another well used convention.

The alternative usage of --switch true and --switch false could also be supported by matching against the switch name after the prefix definition.

@natemcmaster
Copy link
Owner

@JakeSays your suggestion on toggle enum is an interesting idea. I think I would implement it slightly differently though. What do you think of usage that looks like this?

Option<ToggleState> myMappedOption = app.AddMappedOption<ToggleState>(o =>
{
    o.Map("--yes", ToggleState.Yes);
    o.Map("--no", ToggleState.No);
    o.Map("--maybe-so", ToggleState.Maybe);
});

// or with attributes

public class Program
{
    [MappedOption("--yes", ToggleState.Yes)]
    [MappedOption("--no", ToggleState.No)]
    [MappedOption("--maybe-so", ToggleState.Maybe)]
    public ToggleState State { get; set; }
}

@JakeSays
Copy link

JakeSays commented Feb 18, 2020

@natemcmaster Is that mapping a prefix to a state? I am a little unclear on how it would be used.

How would I get from o.Map("--no", ToggleState.No) to --no-unused-parameter and --unused-parameter, or o.Map("--enable", ToggleState.Yes); o.Map("--disable", ToggleState.No) to --enable-touch-screen and --disable-touch-screen?

Also ToggleState.Maybe doesn't make sense to me, given my aversion to ambiguity. What would maybe mean in this context? Typically a prefix-switch defaults to one of yes/no, so the idea behind ToggleState.Unchanged is to indicate the user didn't specify anything, so go with the default. ToggleState.Default would make more sense now that I think about it.

@natemcmaster
Copy link
Owner

Sorry, allow me to clarify. I meant for ToggleState to be something from user code, not in this library. Maybe this makes it clearer?

Option<FruitOption> myMappedOption = app.AddMappedOption<FruitOption>(o =>
{
    o.Map("--banana", FruitOption.Banana);
    o.Map("--apple", FruitOption.Apple);
    o.Map("--pear", FruitOption.Pear);
});

// or with attributes

public class Program
{
    [MappedOption("--banana", FruitOption.Banana)]
    [MappedOption("--apple", FruitOption.Apple)]
    [MappedOption("--pear", FruitOption.Pear)]
    public FruitOption Fruit { get; set; }
}

I think this would be generic enough to support the uses cases above

enum TouchScreen
{
   Enabled,
   Disabled
}

app.AddMappedOption<TouchScreen>(o =>
{
   o.Map("--disable-touch-screen", TouchScreen.Disabled);
   o.Map("--enable-touch-screen", TouchScreen.Enabled);
});

enum MyFeature
{
   Yes,
   No
}

app.AddMappedOption<MyFeature>(o =>
{
   o.Map("--feature", MyFeature.Yes);
   o.Map("--no-feature", MyFeature.No);
});

@JakeSays
Copy link

JakeSays commented Feb 18, 2020

@natemcmaster ahh ok. I see now.

Yes, I think that would work nicely.

The only scenario that isn't covered is --touch-screen=(enabled|disabled), but this could be accomplished with existing functionality. It would mean one more option declaration, but generally speaking I rarely see prefix switches and value switches being used for the same option. It's usually one style or the other. Therefore I personally don't see the need to explicitly accommodate this scenario.

I believe this could also solve #329.

Would it make sense to have AddMappedLongOption() and AddMappedShortOption()?

AddMappedShortOption(o =>
{
    o.Map("-T", TouchScreen.Enabled);
});

I personally wouldn't use it, but someone might.

@natemcmaster
Copy link
Owner

I'd rather just have one API which supports both short and long options.

Option<FruitOption> myMappedOption = app.AddMappedOption<FruitOption>(o =>
{
    o.Map("-b|--banana", FruitOption.Banana);
    o.Map("-a", FruitOption.Apple);
    o.Map("-p|--pear", FruitOption.Pear);
});

@natemcmaster natemcmaster added this to the 3.0 milestone Feb 19, 2020
@JakeSays
Copy link

Ah. Yes of course - makes total sense.

@TheConstructor
Copy link
Contributor Author

While your suggestion would nicely cover the parsing (without another field), I have some questions:

  • is there any idea how to present this in help-output? As it would be defined I could imagine that you could either generically generate a header like
Usage: ConsoleApp1 [options]

Options:
  -i|--int      IntOption
  -?|-h|--help  Show help information

Fruit:
  -b|--banana   Banana
  -a            Apple
  -p|--pear     Pear

or

  (-b|--banana)|-a|(-p|--pear)   Fruit: Banana, Apple, Pear

my preferred way of

  --[no-]fruit  (don't) offer fruit

would probably require either a custom help-provider, or we could 'misue' the outer mapped-option to carry two fields for documentation purposes

  • should the parser handle/allow valued options? Example could be
public class Program
{
    [MappedOption("--banana", FruitOption.Banana)] // default NoValue
    [MappedOption("--apple", FruitOption.Apple, CommandOptionType.SingleValue)]
    [MappedOption("--pear", FruitOption.Pear, CommandOptionType.SingleOrNoValue)]
    public (FruitOption fruit, string color) Fruit { get; }
}
  • Should we offer the bool-first tupples? public (bool valueProvided, FruitOption fruit) / public (bool valueProvided, (FruitOption fruit, string color)) Fruit { get; } or would this be too complicated?

  • Especially if we allow values, we might want to support multiplicity-specification on the group of mapped options i.e. you may want to have a red apple, a green apple and a peach.

@JakeSays
Copy link

@TheConstructor Some thoughts:

(-b|--banana)|-a|(-p|--pear) Fruit: Banana, Apple, Pear - I would hate to require my users to understand regular expression syntax.
--[no-]fruit (don't) offer fruit - the presence or absence of any of the mapped switches would accomplish this need. Having an explicit option like this and the value switches would be confusing.

public (FruitOption fruit, string color) Fruit { get; }

What is the purpose of the above tuple? What would the string represent? If you are considering having both the value switches and --fruit=pear then this again is just more complexity for the end user. I would suggest only one of the two syntaxes.

For this:

Especially if we allow values, we might want to support multiplicity-specification on the group of mapped options i.e. you may want to have a red apple, a green apple and a peach.

It may make sense to base multiplicity off of the property type. If the property is a container then it would implicitly support multiple values.

@natemcmaster
Copy link
Owner

My two cents:

is there any idea how to present this in help-output?

I was planning to start with this for simplicity:

Usage: ConsoleApp1 [options]

Options:
  -i|--int      IntOption
  -?|-h|--help  Show help information
  -b|--banana   Banana
  -a            Apple
  -p|--pear     Pear

We can provide overrides for those who want to customize this more.

should the parser handle/allow valued options?

No, I don't think so. This scenario sounds more like PowerShell's parameter sets, which is a feature of PowerShell that I personally hate and find confusing whenever I've encountered it.

Should we offer the bool-first tupples?

Too complicated.

@natemcmaster natemcmaster removed this from the 3.0 milestone Mar 21, 2020
TheConstructor added a commit to TheConstructor/CommandLineUtils that referenced this issue Sep 25, 2020
Building towards natemcmaster#334 null should only reach this parser for CommandOptionTypes NoValue or SingleOrNoValue - in both cases true is more likely to be desired.
TheConstructor added a commit to TheConstructor/CommandLineUtils that referenced this issue Apr 3, 2021
TheConstructor added a commit to TheConstructor/CommandLineUtils that referenced this issue Apr 3, 2021
TheConstructor added a commit to TheConstructor/CommandLineUtils that referenced this issue Apr 3, 2021
TheConstructor added a commit to TheConstructor/CommandLineUtils that referenced this issue May 25, 2021
TheConstructor added a commit to TheConstructor/CommandLineUtils that referenced this issue May 25, 2021
@stale
Copy link

stale bot commented Jul 21, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please comment if you believe this should remain open, otherwise it will be closed in 14 days. Thank you for your contributions to this project.

@stale stale bot added the closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. label Jul 21, 2021
@TheConstructor
Copy link
Contributor Author

#451 is still work in progress

@stale stale bot removed the closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. label Jul 22, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please comment if you believe this should remain open, otherwise it will be closed in 14 days. Thank you for your contributions to this project.

@github-actions github-actions bot added the stale label Jul 23, 2022
@github-actions
Copy link

github-actions bot commented Aug 7, 2022

Closing due to inactivity.
If you are looking at this issue in the future and think it should be reopened, please make a commented here and mention natemcmaster so he sees the notification.

@github-actions github-actions bot added the closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. label Aug 7, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-stale This issue is closed because it went stale and there was no action planned. It can be reopened. enhancement stale
Projects
None yet
Development

No branches or pull requests

3 participants