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

[Console][RFC] Add named arguments (aka required options) #48244

Closed
wants to merge 3 commits into from

Conversation

c33s
Copy link

@c33s c33s commented Nov 18, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #14716
License MIT
Doc PR symfony/symfony-docs#...

added mode InputOption::REQUIRED to allow required "options" aka "named arguments"

edit:

  • targeted 6.2 branch for now as there is no 6.3 yet
  • definition how the mode should exactly behaves is to be discussed
    • should a required option also require a value? in the sense of named arguments it would speak for having a required value but maybe i miss something.

    • in combination with negatable it would make sense again to not require a value, so you are forced to provide --delete or --no-delete.

    • should it be allowed to require array options?

    • from the logic point of view a required option should not have defaults, should it?

    • new tests have to be fixed/adapted

  • quite many tests are failing which don't look like to have something to do with my change, is this right?

edit2:
renamed named parameters to named arguments

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

c33s and others added 2 commits November 19, 2022 01:16
Co-authored-by: Oskar Stark <oskarstark@googlemail.com>
@OskarStark OskarStark added Console RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Nov 19, 2022
@carsonbot carsonbot changed the title [WIP/RFC] added required options [Console] [WIP/RFC] added required options Nov 19, 2022
@OskarStark OskarStark changed the title [Console] [WIP/RFC] added required options [Console][RFC] Add required options Nov 19, 2022
@xabbuh xabbuh modified the milestones: 6.2, 6.3 Nov 20, 2022
@chalasr
Copy link
Member

chalasr commented Nov 20, 2022

Python supports required options but recommends not using them:

Required options are generally considered bad form because users expect options to be optional, and thus they should be avoided when possible.

For this reason and the ones given in the related issue (tl;dr: not standard-compliant), I'm 👎 on this. Also this can be achieved by throwing some exception.
Thanks for proposing.

@c33s c33s changed the title [Console][RFC] Add required options [Console][RFC] Add named arguments (aka required options) Nov 20, 2022
@c33s
Copy link
Author

c33s commented Nov 20, 2022

@chalasr the name required options is misleading, it is better named "named arguments".

as i started with symfony (v1) most of the classes used getter and setter. the constructor was not used most of the time. calling each setter one after the other really gave a good readability what was set, one line could simply be commented out or switched.

$user
    ->setFirstname("firstname")
    ->setLastName("lastname")
    ->setAge("99")
    ->setPassword("password")
    ->setEmail("foo@example.com")
;

switching to immutable objects, with no setter and only a constructor to set the initial state of the object, made a lot of sense but really made the creation more error-prone. having multiple string parameters in a constructor was quite dangerous and it was easy to mix up the order of the parameters.

$user = new User("lastname", "firstname", 99, "foo@example.com", "password); //oops switched firstname with lastname.

the really great solution from php was the introduction of named parameters. explicit naming the parameter prevents errors.

$user = new User(
    lastname: "lastname", 
    firstname: "firstname", 
    age: 99, 
    password: "password", 
    email: "foo@example.com"
);

the same is with associative config arrays. we are using them instead accesing values from an array by index.

public const OPTIONS_DEFAULTS = [
    null,
    null,
    [],
    [],
    '',
    null,
    null,
    20,
    null,
];

vs

public const OPTIONS_DEFAULTS = [
    'auth_basic' => null,
    'auth_bearer' => null,
    'query' => [],
    'headers' => [],
    'body' => '',
    'json' => null,
    'user_data' => null,
    'max_redirects' => 20,
    'http_version' => null,
];

so why exactly is it a bad practice to have named arguments in the cli? it is ok to have unnamed arguments if you have only one or two arguments but having 3+ requires you checking the right order most of the time.

bin/console add:user <firstname> <lastname> <age> <password> <email> --force

vs

bin/console add:user --firstname=<firstname> --lastname=<lastname> --age=<age> --password=<password> --email=<email> --force
  • it is clearer what is what
  • you can easily change the order of the priovided arguments without creating a BC break.

with php now having named parameters i thought it would be good thing to also have the option (nobody has to use it if they don't want to) of named arguments would be a good thing.

maybe naming the constant REQUIRED_ARGUMENT would make the semantics better? it is not that much extra code to maintain and the code is quite clear and nearly identical to the required arguments.

@c33s
Copy link
Author

c33s commented Nov 26, 2022

just stumbled over another usecase: add multiple users to multiple projects. at least one username and one project is required.

quote from the symfony docs:

Only the last argument can be a list

so doing it with two InputArgument::IS_ARRAY is not possible.

bin/console add:users username_1 username_2 ... username_n project_1 project_2 ... project_n

we need to set users to InputArgument::IS_ARRAY and projects to InputOption::VALUE_IS_ARRAY (or the other way around) but this also means it the project option is not optional because i have to supply at least one project.

bin/console add:users username_1 username_2 ... username_n --project=project_1 --project=project_2 ... --project=project_n

as workaround i can only switch to 2 string arguments and do a coma-separation and explode.

bin/console add:users username_1,username_2,...,username_n project_1,project_2,project_n

or i can add the required option code myself.

i think it would really make sense to have the feature "named arguments" in the core. real world use cases are there.

holding onto the semantic of the word "option" instead of going with the words "named argument" is just withholding the desired path.

desire-paths-pics-203-6351402d44eaa__700
source: reddit.com

i found one interesting article Desire paths: the unofficial footpaths that frustrate, captivate campus planners about this topic and a collection of images 50 Times City Architects Failed To Understand People’s Needs, And It Resulted In These ‘Desire Paths’ Appearing Around The City .

@chalasr
Copy link
Member

chalasr commented Dec 5, 2022

I'm still not convinced we need this given that the above use cases can be achieved with some custom code in userland and this is not supported by most relevant standards. Calling @symfony/mergers for additional opinion(s)

@lyrixx
Copy link
Member

lyrixx commented Dec 5, 2022

To me, an option should always be optional. So I'm against required options.

I agree with @chalasr, it can still be achieve in userland.

@chalasr
Copy link
Member

chalasr commented Dec 6, 2022

Closing, thanks for pushing the discussion.

@chalasr chalasr closed this Dec 6, 2022
@berniedurfee-renaissance

Doesn't VALUE_REQUIRED imply that the option must be set? If the option is not set, there's no value, thus the required value is missing. Right?

@stof
Copy link
Member

stof commented May 11, 2023

No. It means that if you set the option, it is required to provide a value (that's why it is named InputOption::VALUE_REQUIRED, not REQUIRED, unlike InputArgument::REQUIRED).

@PeterMarteau
Copy link

I know it's an old discussion, but i stumbled across this myself as i wanted to use a named argument (as it's required it's an argument) or a required option (as its key is prefixed with either one or two hyphens). And i do think a named arguments are a valid use-case.

Since i didn't find the arguments made here completely convincing, i had a look. To spare anyone the time to do it themselves, here's what i think i found:

There were basically two arguments made against it:

  1. that it puzzles the user because of the name (an option is optional)
  2. and that it was (implicitly therefore) not part of any standard.

I'm not familiar with writing console commands in linux, but if i am not mistaken, argument 2 is wrong: It is actually part of the only relevant standard: POSIX 12.1 under list number 9:

The form:

utility_name -f option_argument [-f option_argument]... [operand...]

indicates that the -f option is required to appear at least once and may appear multiple times.

The only command i found that has "required options"/"named arguments" was "tar", but i wouldnt consider it an essential part of linux and even if it was, it could've and should've been done another way imo.

"Required options" being part of POSIX of course doesn't mean, that it isn't confusing. But the problem is not that it is not part of any standard, but that the implementation of named arguments in the POSIX standard is done in a confusing way: It includes "named arguments" (functionality-wise) in a misleading way as "required options", because of its hyphen prefixed syntax, which makes it a mix of an argument and an option. My best guess is that they wanted to include named arguments, but defining a new syntax was more complex than just ignoring semantics a bit (probably by saying: this is a required option, because it has "options to choose from"):

  1. ++ or --- as prefix would add too much complexity (++) and/or still be confusing (---)
  2. adding = after an argument would probably not be compatible with existing arguments (that may have included "=" as part of the value)

I think that's also the reason why python includes and discourages it at the same time.

Of course i can be completely wrong about all this, but it makes perfect sense to me.

@adriangligor
Copy link

adriangligor commented Apr 28, 2024

I've been struggling to remember the order of arguments for ln -s my whole life :) Therefore I second any attempt to support named arguments/parameters (I've never called them "options" -- so no semantic conflict).

As an example from the coding world, IntelliJ helpfully shows parameter names inline in code because users found it so helpful with complex function calls:
Screenshot 2024-04-28 at 10 02 34

Calls to Symfony commands often end up further upstream, in shell scripts or CI/CD pipeline code. There, variables are often used to provide values to the arguments, making the resulting command difficult to understand, e.g.:

bin/console release:tag-in-sentry $CI_COMMIT_SHORT_SHA $CI_COMMIT_TAG $CI_JOB_ID $CI_COMMIT_TIMESTAMP

I find it tiresome figuring out later whether the job ID goes into the description or the tag argument of this particular Sentry release. Also, adding a fifth argument at the third position later is very error prone. When working with such code, one always needs to have the source of the Symfony command at hand, which is tough on our DevOps guys since they don't really know PHP, especially not the DI-heavy object-oriented kind.
(note: what sentry.io is and does is not relevant to this argument)

So consider this a 👍 from me.

@jumpski
Copy link

jumpski commented May 21, 2024

A thumbs up from me too. 👍

Almost everyone knows it, especially on stressful days, the parameters don't quite work the way you're used to. Support like this is very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Console Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console][Feature Request] Required options