8000 feat: #43086 add blacklisting of reserved shortcuts for InputOptions in commands by Chris53897 · Pull Request #43091 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

feat: #43086 add blacklisting of reserved shortcuts for InputOptions in commands #43091

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

feat: #43086 add blacklisting of reserved shortcuts for InputOptions in commands #43091

wants to merge 9 commits into from

Conversation

Chris53897
Copy link
Contributor
Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #43086
License MIT
Doc PR symfony/symfony-docs#...

This is an idea how to solve #43086
The blacklisting of reserved shortcuts for InputOptions in commands is done on initialisation and not at runtime.
I do not like the fact of the hardcodet arrays.
It would be better to read them from getDefaultInputDefinition() in Symfony\Component\Console\Application. If possible.

There is space for more improvements. Please share your ideas.

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@Chris53897 Chris53897 marked this pull request as ready for review September 19, 2021 09:06
@Chris53897 Chris53897 requested a review from chalasr as a code owner September 19, 2021 09:06
@Chris53897 Chris53897 requested a review from xabbuh as a code owner September 19, 2021 10:20
@Chris53897
Copy link
Contributor Author
Chris53897 commented Sep 19, 2021 •
edited
Loading

Adjust Tests:
The test testAddingOptionWithDuplicateShortcut fails.
Because this PR is raises the error for blacklisted shortcuts before runtime.
So i changed the test to a non blacklisted shortcut "t".
I changed testAddingAlreadySetDefinitionElementData($def) there is an InvalidArgumentException now instead of a LogicException if an blacklisted shortcut is used.
Additional the runtime-component test-command used the reserved shortcut "e". So i changed that too.

The [actual error from PHPUnit ](https://github.com/symfony/symfony/pull/43091/checks?check_run_id=3644332026)should be gone, after this is merged. #43088
So i made the small fix in this PR to.

There is a conflict with text indent to let the test pass and Code Standard from fabbot.io
I changed the test according to the other tests.

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Sep 19, 2021
@carsonbot
Copy link

Hey!

I think @yoannrenard has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@fabpot fabpot modified the milestones: 5.4, 6.1 Oct 29, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@nicolas-grekas
Copy link
Member

I feel like this is solving a problem that is not worth solving.
Hardcoding reserved names looks quite arbitrary to me (the reserved names are).
Also, this didn't get any traction is almost a year.
I'm therefore closing. Thanks for submitting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

Assignees
No one assigned
Labels
Deprecation Feature Status: Needs Review
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

[Console] list-command parameter --format : An option with shortcut "e" already exists.
4 participants
@Chris53897 @carsonbot @nicolas-grekas @fabpot
0