10000 [Console] Added three state long option by ocke · Pull Request #11883 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] Added three state long option #11883

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[Console] Added three state long option #11883

wants to merge 9 commits into from

Conversation

ocke
Copy link
@ocke ocke commented Sep 8, 2014

[Console] Added three state long option

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11572
License MIT
Doc PR

A three state long option, instead of the current 2 state version:

  • Not setting the longoption will make the default set to false, just like a standard longoption.
  • Setting the longoption without a value will set it to it's default
  • Setting the longoption with a value will set it to that value.

Let's say --dev

php tool.php
array(1) {
  'dev' =>
  bool(false)
}
php tool.php --dev
array(1) {
  'dev' =>
  'the default value'
}
php tool.php --dev=test
array(1) {
  'dev' =>
  'test'
}

A couple of notes:

  • I assume you guys agree that it's not a good idea to break current functionality. Therefor VALUE_OPTIONAL will stay how it is and I added a new constant: VALUE_OPTIONULL.
  • All the tests still work!
  • We need to come up with a better name.
  • I'll need to write new test for the new config option (VALUE_OPTIONULL).

@ocke ocke mentioned this pull request Sep 8, 2014
Closed
@ocke ocke changed the title Possible solution to 3 state long options [Console] Added three state long option Sep 8, 2014
Nicofuma
Nicofuma reviewed Sep 9, 2014
View reviewed changes
src/Symfony/Component/Console/Input/InputOption.php

private $name;
private $shortcut;
private $mode;
private $default;
private $set_default;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference with $default?

Copy link
Author

Choose a reason for hiding this comment

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

$default is the 'true' default value when a long option is not defined. It will always be set to false for long_options with VALUE_OPTIONULL. Just like it will always be set to false for VALUE_NONE. $set_default is the default used when an option is used but doesn't have a value set (i.e. --dev).

The problem is that InputOption is unaware of which default to use ('foo' vs 'foo --dev' vs 'foo --dev=bar') because it doesn't know the arguments the user passed.

Which resulted in me adding some smarts to InputDefinition to determine which 'default' to use. Which in turn means I need to keep both 'defaults' in InputOption.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it should be written somewhere, the difference has to be clearly defined in the docblocks of the setters.

@stof
Copy link
Member
stof commented Sep 9, 2014

anyone with a better name ? OPTIONULL is not an existing word so it is not a good name

@yguedidi
Copy link
Contributor
yguedidi commented Sep 9, 2014

VALUE_TERNARY?

@ocke
Copy link
Author
ocke commented Sep 10, 2014

@stof, @yguedidi
have a look at what I produced. Let me know if you have any ideas about it 👍

stof
stof reviewed Sep 10, 2014
View reviewed changes
src/Symfony/Component/Console/Input/InputOption.php
* Sets the default value.
*
* For VALUE_TERNARY the $default will always be set to false (for when the longoption is not used)
Copy link
Member

Choose a reason for hiding this comment

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

missing space: long option

Copy link
Member

Choose a reason for hiding this comment

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

btw, why talking about long option here ? It is true for shortcuts too

Copy link
Author

Choose a reason for hiding this comment

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

oh ok, I'll fix that

@stof
Copy link
Member
stof commented Sep 10, 2014

Tests are missing for this feature

@ocke
Copy link
Author
ocke commented Sep 10, 2014

Yes. I will start writing the tests now :-)

@yguedidi
Copy link
Contributor

I don't like setDefault.. What about something like flagValue ? As it is the value when the option is used as a flag.

@ocke
Copy link
Author
ocke commented Sep 10, 2014

Good point. I will change that. I'm also not very happy with the current description that I added.
Also there is not setSetDefault (or that would become setFlagValue)... should I add that?

@stof stof added the Console label Sep 20, 2014
@ocke
Copy link
Author
ocke commented Sep 23, 2014

@stof, @yguedidi
Any additional code I need to add or last advice?

You guys think this solution is a go or a no go?

@OndraM
Copy link
Contributor
OndraM commented Jan 3, 2015

Personally, I prefer solution from this PR: #12773. It could be used for the same use-case, but it has minimal impact on other code. The only disadvantage is that it may not be as straightforward as using dedicated option type.

Example usage: #12769 (comment)

@ocke
Copy link
Author
ocke commented Jan 5, 2015

I leave it up to you guys to decide what's best. I think my solution works great, the tests show it works, but it obviously involves more code changes. It's there for you guys to use, or not.

@wtfred
Copy link
wtfred commented Jan 16, 2015

Hi guys, looking forward for this to be merged, i just start using Console and ran directly into this problem :s

@OskarStark
Copy link
Contributor

any news here?

The PR: #12769 is closed

@fabpot
Copy link
Member
fabpot commented Oct 2, 2015

Closing as #12773 make it work without having to add another configuration setting.

@fabpot fabpot closed this Oct 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Console
Projects
4C5D None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ocke @stof @yguedidi @OndraM @wtfred @OskarStark @fabpot @Nicofuma
0