Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Console]#[Option]
rules & restrictions#59602
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
InputOption::VALUE_OPTIONAL
InputOption::VALUE_OPTIONAL
GromNaN commentedJan 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There are also cases where the parameter doesn't have a default value, or where the default value is inconsistent.
|
kbond commentedJan 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I suggest we make options require a default in their argument definition:
I think, right now, that would be |
@kbond for a no-op boolean flag, you still need to keep the same default value than before, because you still need to detect the explicit usage of the flag to display deprecation warning (but you would then ignore that flag for the actual logic) |
@stof, wouldn't |
chalasr commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree, better expose a negatable option for such cases and then the
Not sure as using
Same as above, I think it should be
No? And
Makes sense to me. |
Thanks for the review!
I'm sure it's fine but it isn't crystal clear that |
What about I sort of thought it was easier to reason about if we have a hard "all options require a default" rule. |
chalasr commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No blocker but I feel like it is clear enough that
I think yes.
And I don't strongly disagree, I'm just leaning towards leaving this up to implementors as I can't think of any issue it could cause and one may be happy to lighten their signatures.
If by appended you mean that you end up with the default values + the user values in the array then I would say no. As soon as one set such array option once the default is lost IMHO |
kbond commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ok, I'm good with this, couple of remaining questions, what should the following do?
|
GromNaN commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
If there is some combination of parameter type, default value and input type that seems illogical, we can throw a LogicException. |
What about no default value means negatable, default means not? @GromNaN I’m not sure to understand, could you illustrate? |
kbond commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That's kind of what I'm trying to do here: detailing things that don't make sense and throw logic exceptions.
I'm ok with that. But when there's no default ( |
chalasr commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Default false (when omitted) matches with what |
That all sounds good. I'll adjust this PR to enforce/accommodate these rules (hopefully in a way that makes it clear). |
Hey there! here are my thoughts about some points
|
|
This was my thinking also - you articulated it better 😉
How else could we allow negatable options (where you need to know if it was explicitly passed)?
Again, how else could we handle this case? |
Forget that comment, I just saw that it's the current case, and allow to check the option was explicitly passed. Nothing new there regarding the current implementation, right? |
yceruto commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Here IMO it has a different meaning, a nullable option should not require a value (seeing it from the usage perspective). I think this case can be currently handled with |
I don't believe so but I'll double check.
That's fair. I guess the only thing I don't like is not being able to distinguish betweenno option passed and |
yceruto commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
that case is actually tested in |
I don’t see a real scenario either, but you can still use |
yceruto commentedMay 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I’d like to revisit these general rules from a usage perspective, not because they’re ideal, but because they reflect how we’ve implemented the current behavior:
|
yceruto commentedMay 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Found some signatures that probably deserve a
|
Had to stare at 3 for a while but yes, that all sounds good to me. |
Thanks for all the feedback! I'll finalize this PR this week. |
cfe06ba
to06c2ece
CompareInputOption::VALUE_OPTIONAL
#[Option]
rules & restrictionskbond commentedMay 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ok, PR updated:
There's likely other logic checks we could make but I didn't want to go overboard.
I've discovered this is just how |
Uh oh!
There was an error while loading.Please reload this page.
06c2ece
to41a5462
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM! thanks Kevin!
Thank you@kbond. |
e532750
intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
This PR was merged into the 7.3 branch.Discussion----------[Console] `#[Option]` rules & restrictions| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | no| New feature? | no| Deprecations? | no| Issues | n/a| License | MIT1. Adds tests for the rules described in [this table](symfony/symfony#59602 (comment))2. Adds exceptions/tests for the restrictions described in [this table](symfony/symfony#59602 (comment))Commits-------41a5462f0d4 [Console] `#[Option]` rules & restrictions
I just upgraded to beta-2 and wanted to give feedback as now most of my CLI commands won't even compile anymore, because I depend on null strings in options. For example, I have a default transport that in my dev environment is set to sync, in production it is async. I inject that information in the contructor, and then use it in the commen publicfunction__construct( #[Autowire('%env(DEFAULT_TRANSPORT)%')]private ?string$defaultTransport =null, ) { }publicfunction__invoke(SymfonyStyle$io, #[Option(description:'message transport')] ?string$transport =null, ) {$transport ?==$this->defaultTransport; } Similarly, I rely on null to mean that an option wasn't pass it, so I can prompt for it. if ($transition) {assert(in_array($transition,$transitions),"invalid transition:\n\n$transition: use\n\n" .join("\n",$transitions)); }else {$question =newChoiceQuestion('Transition?',$transitions,0 );$transition =$io->askQuestion($question); } This worked fine in zenstruck/console-extra and beta1, and I use it all the time. publicfunction __invoke(SymfonyStyle$io, #[Argument(description:'class name')] ?string$className =null,# to override the default #[Option(description:'message transport')] ?string$transport =null, #[Option(description:'workflow transition')] ?string$transition =null, #[Option(name:'workflow', description:'workflow (if multiple on class)')] ?string$workflowName =null,// marking CAN be null, which is why we should set it when inserting #[Option(description:'workflow marking')] ?string$marking =null, #[Option(description:'tags (for listeners)')] ?string$tags =null, #[Option(name:'index', description:'grid:index after flush?')] ?bool$indexAfterFlush =false, #[Option(description:'show stats only')] ?bool$stats =false, #[Option]int$limit =0, #[Option(description:"use this count for progressBar")]int$count =0, #[Option]string$dump ='', ):int { I realize I could set all the string options to blank, but I use null to mean that nothing was pass it (the option wasn't set). I realize nullable booleans have their own meaning (because you can have --no-option and --option), but a nullable string seems like it should mean the option wasn't passed at all. |
chalasr commentedMay 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@tacman thanks for testing beta features. Quoting#59602 (comment):
That's why currently using an empty string as default for such options is the way to go. Maybe this would make sense and help though:
? Not sure it's already supported. |
I tend to agree with@tacman - I think it'll be common to want
The only hesitation I have it it'd be super similar to
|
I agree. To avoid the ambiguity, what about enforcing |
I tried this but because of how |
Hmm, actually, maybe I can adjust the value here:
I can give this another try |
This PR was merged into the 7.3 branch.Discussion----------[Console] Invokable command `#[Option]` adjustments| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | no| New feature? | yes| Deprecations? | no| Issues |Fix#59602 (comment)| License | MITThe *only* way to make an option `VALUE_OPTIONAL`.| Signature | Definition | Usage | Value ||--|--|--|--|| `string\|bool $opt = false` | `VALUE_OPTIONAL` | `<omitted>` <br> `--opt=val` <br> `--opt` | `false` (default)<br>`"val"` <br>`true` |Now throws an exception:| Signature | Definition | Usage | Value ||--|--|--|--|| ~`?string $opt = ''`~ | ~`VALUE_OPTIONAL`~ | ~`<omitted>` <br> `--opt=val` <br> `--opt`~ | ~`''` (default)<br>`"val"` <br>`null`~ |Now is valid:| Signature | Definition | Usage | Value ||--|--|--|--|| `?string $opt = null` | `VALUE_REQUIRED` | `<omitted>` <br> `--opt=val` | `null` (default)<br>`"val"` || `?array $opt = null` | `VALUE_ARRAY` & `VALUE_REQUIRED` | `<omitted>` <br> `--opt=val1` | `null` (default)<br>`['val1']` |Commits-------59a4ae9 [Console] Invokable command `#[Option]` adjustments
Uh oh!
There was an error while loading.Please reload this page.