Skip to content

[Console] Added more verbosity levels #7626

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

Closed
wants to merge 5 commits into from

Conversation

dlsniper
Copy link
Contributor

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

This adds new verbosity levels.

@dlsniper
Copy link
Contributor Author

From the original proposal of @Seldaek --verbose=1/2/3 (with --verbose being equivalent to --verbose=1) and -v/-vv/-vvv I don't know how we can do the shorthand versions, the -vv and -vvv to be more accurate as the current Console component doesn't support multiple aliases.

@fabpot do we want to support multiple aliases? If so, I can do the patch as well.

If the PR is ok, I'll create the doc PR for this to be mergeable.

}

if (true === $input->hasParameterOption('-vvv')) {
$output->setVerbosity(OutputInterface::VERBOSITY_VERY_VERBOSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be DEBUG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've forgot to remove them from the commit as they are not used right now, see the comment above. Thanks for pointing them out.

@fabpot
Copy link
Member

fabpot commented Apr 11, 2013

@dlsniper Support for multiple aliases as you call them should be done in another PR.

@Seldaek
Copy link
Member

Seldaek commented Apr 11, 2013

The PR is not mergeable yet IMO, it adds weird bugs. I'm investigating.

@dlsniper
Copy link
Contributor Author

Thanks for feedback, I'm travelling right now, I'll fix the remaining issues tonight.

@Seldaek
Copy link
Member

Seldaek commented Apr 11, 2013

Sent a PR (dlsniper#2) with some fixes, but that's unfortunately not the end of it. Since --verbose now has an optional value, doing things like: composer show --verbose symfony/symfony fails, because it takes symfony/symfony as the value to --verbose, and not as an InputArgument as it was before. Similarly doing composer --verbose 2 show symfony/symfony fails for example, because the parser takes the first argument as the command name, in this case it's 2 so it fails hard.

I see a few ways of dealing with this:

  • revert the --verbose option to be VALUE_NONE. It still works as expected since we inspect the input tokens directly with hasParameterOption, and it fixes the problems. However that means you can't do --verbose 3 anymore, you need the =.
  • make some ugly hack in the input parsing to ignore values following --verbose if they aren't 1/2/3
  • add some new thing in the InputOption class to specify accepted values, so that non accepted ones are ignored if a default value is provided. It's equally hackish IMO but makes it look more legit.

I think I would prefer the first approach, because it's easy, and introduces a minor inconsistency for the verbose flag but that's hopefully ok.

@dlsniper
Copy link
Contributor Author

@Seldaek I've fixed the remaining bugs and I've used the first option. It's not really pretty but it works so it should be fine from my pov. Can you check if it works for you now?

I've also changed the text to the one suggested by @fabpot

If everything looks good, I'll do the doc PR asap.

I'll start with the PR for multiple shortcuts/aliases if we want to have support for that as well, let me know about it.

@Seldaek
Copy link
Member

Seldaek commented Apr 11, 2013

Answered inline in new commits regarding the code.

I'll start with the PR for multiple shortcuts/aliases if we want to have
support for that as well, let me know about it.

I don't think multiple aliases really makes sense in most cases. Might
be good to have it for completeness, but usually you don't need to add
more mess to the cli args ;) And in this case it's not really aliases,
because -vv is different from -v.

@fabpot
Copy link
Member

fabpot commented Apr 20, 2013

What the status of this PR?

@dlsniper
Copy link
Contributor Author

@fabpot sorry for not responding earlier. I don't have the time to make this feature work in time for 2.3. If someone else wants to take over it, please do so. Feel free to close this when a new PR appears.

@Seldaek
Copy link
Member

Seldaek commented Apr 22, 2013

@dlsniper I'll try to take over because I really need this.

@fabpot
Copy link
Member

fabpot commented Apr 25, 2013

Closing in favor of #7841

@fabpot fabpot closed this Apr 25, 2013
fabpot added a commit that referenced this pull request Apr 25, 2013
This PR was merged into the master branch.

Discussion
----------

[Console] Add more verbosity levels

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6066
| License       | MIT
| Doc PR        | symfony/symfony-docs#2554

This adds new verbosity levels.

Replaces #7626 /cc @dlsniper

Commits
-------

02c2038 Fix tests
5327ec4 Add notes in UPGRADE and CHANGELOG
77c9791 [Console] Add tests and support for more types of inputs
54c1377 [Console] Handle new verbosity levels
e639686 Revert invalid changes
8f4d376 Removed unused options
5bb4163 Fixed tests
b62d35f Fix handling of --verbose=... and BC break
16cdb61 Added more verbosity levels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants