Skip to content

[Console] Different approach on merging application definition #20054

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
Merged
merged 1 commit into from
Aug 13, 2020
Merged

[Console] Different approach on merging application definition #20054

merged 1 commit into from
Aug 13, 2020

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Sep 25, 2016 •
edited
Loading

Q A
Branch? "master"
Bug fix? yes
New feature? not really (refactoring)
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #19181, #17804, #19909, partially #20030
License MIT
Doc PR reference to the documentation PR, if any

Before/After:

$ bin/console list -h
Usage:
  list [options] [--] [<namespace>]

Arguments:
  namespace            The namespace name

Options:
      --raw            To output raw command list
      --format=FORMAT  The output format (txt, xml, json, or md) [default: "txt"]
+  -h, --help            Display this help message
+  -q, --quiet           Do not output any message
+  -V, --version         Display this application version
+      --ansi            Force ANSI output
+      --no-ansi         Disable ANSI output
+  -n, --no-interaction  Do not ask any interactive question
+  -e, --env=ENV         The environment name [default: "dev"]
+      --no-debug        Switches off debug mode
+  -v|vv|vvv, --verbose  Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug

Help:
  The list command lists all commands:

    php bin/console list

  You can also display the commands for a specific namespace:

    php bin/console list test

  You can also output the information in other formats by using the --format option:

    php bin/console list --format=xml

  It's also possible to get raw list of commands (useful for embedding command runner):

    php bin/console list --raw

This could deprecate getNativeDefinition or make it a feature as right now it's internal and unused.

edit: resolved the BC break.

edit2: question is.. should this target 2.7?

@ro0NL ro0NL mentioned this pull request Sep 27, 2016
Closed
@ro0NL
Copy link
Contributor Author

ro0NL commented Sep 28, 2016 •
edited
Loading

So.. any thoughts on this? I think this is way more obvious than the current approach.

The list command not giving the application-wide options is a bug.. right? However they were more or less explicitly left out... any opinion on this?

edit: yeah.. after #20090 and rebase 2.7 imo.

@ro0NL ro0NL mentioned this pull request Sep 29, 2016
Closed
@fabpot fabpot mentioned this pull request Oct 5, 2016
Closed
@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016
@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 29, 2016 •
edited
Loading

Not sure about intentions here. It's done rather explicitly. The global options do work though with list.

@ro0NL ro0NL closed this Dec 29, 2016
@ro0NL ro0NL deleted the console/command-arguments branch December 29, 2016 09:50
@nicolas-grekas nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017
@ro0NL ro0NL mentioned this pull request Apr 17, 2017
Closed
@ro0NL ro0NL mentioned this pull request Nov 25, 2018
Closed
@l-vo
Copy link
Contributor

l-vo commented Nov 29, 2018

Tested the changes on 3.4 and this approach seems to be working properly.

l-vo
l-vo reviewed Nov 29, 2018
View reviewed changes
@ro0NL ro0NL restored the console/command-arguments branch November 30, 2018 07:36
@ro0NL ro0NL reopened this Nov 30, 2018
@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 30, 2018

status: needs work

@chalasr chalasr modified the milestones: 3.3, next Aug 27, 2019
@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

@ro0NL I'm digging the old PRs right now and stumbled on this one. I see that you marked it as "Needs work", is it still relevant?

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 11, 2020

@fabpot i'd say yes. It boils down to

bin/console list -h
# does NOT show global options

bin/console about -h
# does show global options

bin/console help -h
# does show global options
# includes an arbitrary "command" argument, e.g.;
#
#   Arguments:
#     command               The command to execute
#     command_name          The command name [default: "help"]

Not sure why i marked it "needs work" 😅 i figure to come to some sort of conslusion here.

@fabpot
Copy link
Member

fabpot commented Aug 11, 2020

Ok, can you rebase on current master? I will test it on a real application to see how it behaves.

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 11, 2020 •
edited
Loading

@fabpot should be good. Only need to update fixture files still (please do 😂)

@ro0NL
Copy link
Contributor Author

ro0NL commented Aug 12, 2020

@fabpot console component passes tests 👍

fabpot
fabpot approved these changes Aug 13, 2020
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Great job!

@fabpot
Copy link
Member

fabpot commented Aug 13, 2020

Thank you @ro0NL.

@fabpot fabpot mentioned this pull request Aug 13, 2020
Closed
@fabpot fabpot merged commit a45428c into symfony:master Aug 13, 2020
@ro0NL ro0NL deleted the console/command-arguments branch September 8, 2020 16:51
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l-vo l-vo l-vo left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
BC Break Bug Console Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
6 participants
@ro0NL @l-vo @fabpot @nicolas-grekas @chalasr @carsonbot