gobo-eiffel / gobo

The Gobo Eiffel Project provides the Eiffel community with free and portable Eiffel tools and libraries.
https://sourceforge.net/projects/gobo-eiffel/
Other
59 stars 24 forks source link

Default parameter change to forbid short form makes no sense #36

Closed berenddeboer closed 5 years ago

berenddeboer commented 5 years ago

Revisiting some old code, I see that AP_OPTION_WITH_PARAMETER forbids using the short form of the parameter. That makes no sense to me.

Why does it matter if I use:

   ls -r test

or

   ls --region test

?

ebezault commented 5 years ago

But in you example, -r and --region are not options with parameters. They are flags. In other words, test is not a parameter of the -r. It's a parameter of the command ls.

berenddeboer commented 5 years ago

No it's not. test is the value for the region. Apologies for using ls, that makes it unclear. I should have used s3ls or something like that. I wasn't trying to say anything about the ls command.

ebezault commented 5 years ago

In that case, shouldn't it be --region=test, an not --region test?

ebezault commented 5 years ago

I guess that this is what you are referring to:

set_default_parameter (a_parameter: like default_parameter)
        -- Set the parameter as optional. If no parameter is given,
        -- the corresponding parameter value is set to `default_parameter'.
        -- This only works for long forms and makes it impossible to
        -- specify the parameter as `--option parameter'.
    require
        not_short_form: not has_short_form
ebezault commented 5 years ago

Do you have an issue without calling set_default_parameter?

If you call this routine, then the limitation is a workaround for a parsing ambiguity.

berenddeboer commented 5 years ago

I think you can leave out the '=' isn't it? At least I never pass that. But yes, either way, in my case --region=test or --region test have the same meaning. I think that's pretty standard behaviour.

The reason to ask for this is that it would make for less code. Now I need one more feature, and an if statement to assign either the parameter or its default value to it.

ebezault commented 5 years ago

There is something I don't understand. Allowing for a default parameter in an option was not in the original design of this library. I added it. And when I added it I put the restrictions specified in the header comment of set_default_parameter. So the behavior has not changed and your old code should work the same.

The reason why it says that a short form or a long form without the = sign is not allowed is that when you write ls -r test or ls --region test, it's not clear whether you mean ls | -r | test or ls | -r test, and ls | --region | test or ls | --region test. I got caught by this ambiguity in my first reply. There is no such ambiguity when you don't use set_default_parameter because the parameter test has to be attached to -r or --region.

Now, if ls --region test is parsed as ls | --region test and not ls | --region | test as you seem to say, then it was not intentional in the design of set_default_parameter. So I say that it's a bug.

ebezault commented 5 years ago

Or perhaps you meant to have ls --region test to be interpreted as ls | --region test and use ls --region -- test if we want ls | --region | test. That makes sense. I'll see if I can make it work like that.

berenddeboer commented 5 years ago

I think I submitted the patch for the default parameter :-)

I don't get: ls --region -- test. That should not work I think? --region must have a parameter, so this is a parse failure. How I read this is that --region simply consumes the next word as its parameter.

ebezault commented 5 years ago

That's the whole point about using set_default_parameter: you don't need to pass a parameter to this option. If you don't pass it, then it's as if you passed the value passed as argument of set_default_parameter. If you want the parameter to be compulsory, just don't call set_default_parameter.

I don't see your patch. Where is it?

berenddeboer commented 5 years ago

Didn't create a patch, I meant that I added the default option in the first place.

But I see we have different views about the option: for me the default value applies when you don't specify the parameter at all. You are saying you still have to specify the parameter, but not the value.

That's not what I intended when I created this option. The effect I want to have is this:

  1. Set region parameter default to 'us-east-1'.
  2. User does not have to specify --region us-east-1.
  3. Just having --region is a parsing error.
  4. If option not specified, the value of this option is still us-east-1 as that was the default.

I don't believe I have ever seen any program doing what you suggested, i.e. that you specify the option but without the value.

ebezault commented 5 years ago

OK, I see. What you want is this behavior:

    if option.was_found then
        value := option.parameter
    else
        values := "us-east-1"
    end

but without having to write this code in the clients.

Perhaps what I suggest is not commonly used, but it's used in the Gobo tools. There is an option --ise which tells the tool to follow the Eiffel rules as implemented in EiffelStudio. But one can specify --ise=17.05 if one wants the Eiffel rules as of EiffelStudio 17.05. So when the option --ise is specified with no parameter, it means "use the latest version of EiffelStudio, whatever that version is". This is the default value for this option when it is specified but without any explicit parameter. It also allows to have options of the form --flat=true and --flat=false, and still have --flat as a shortcut for --flat=true. This avoids having two flags --flat and --noflat as is often seen in Unix tools.

I think that it's possible to implement both behaviors in class AP_OPTION_WITH_PARAMETER. We could have set_default_parameter which does what you want, and set_parameter_optional which does what I want. I'll try to implement that.

berenddeboer commented 5 years ago

Ah great, thanks a lot!

ebezault commented 5 years ago

It's now implemented in the master branch.