Navigation Menu

Skip to content
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

[Console] Lazy commands, separate IO concerns, add forced resolve to command, and prittify and detail output #981

Closed

Conversation

robfrawley
Copy link
Collaborator

@robfrawley robfrawley commented Aug 30, 2017

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

This is the 2.0 version of #967. Included are commands with the following new features:

  • Use of commands as a service
  • Removal of "ContainerAwareInterface" from commands (instead, the container is used to pass required dependencies)
  • Updated styling that includes colored output, as well as the ability to disable colors and print machine-readable output
  • Separation of concerns between the commands, the IO/style, etc
  • Enable comparable output for the remove cache command

The use of explicit dependencies (via the constructor), as well as the separation of concerns, results in relatively simple commands, despite the complex behavior and output styling introduced. See:

It is also worth noting that the bulk of the PR size is new and updated unit tests. The new command output for the cache resolve command looks like the following:

imagine-cli-resolve_cached-resolved-failed_normal
imagine-cli-resolve_cached-resolved-failed_no-colors
imagine-cli-resolve_cached-resolved-failed_as-script
imagine-cli-resolve_cached-resolved_normal
imagine-cli-resolve_failed_all

- added "--force" option to resolve command to force re-resolution regardless of cache state
- added "--no-colors" options to output non-styled varient
- added "--as-script" options to output in machine-readable varient
- removed deprecated "--filters" options ("--filter" replaces it)
- removed container-aware implementation of commands for explicit constructor-based dependencies
- added commands as services in "Resources/config/imagine.xml"
- added commands as services lazy loading functionality (on supported Symfony vers) via "command" tag
- added new console style abstraction "Liip\ImagineBundle\Component\Console\Style\ImagineStyle"
- rewrote/refactored command implementation (for cleanup and better organization)
- pulled out common functionality between commands into "Liip\ImagineBundle\Command\CacheCommandTrait"
@robfrawley robfrawley added Level: Enhancement ✨ This item involves an enhancement to existing functionality. State: Reviewing This item is being reviewed to determine if it should be accepted. labels Aug 30, 2017
@robfrawley robfrawley added this to the 2.0.0 milestone Aug 30, 2017
@robfrawley robfrawley self-assigned this Aug 30, 2017
@robfrawley robfrawley changed the title Feature lazy rewritten commands [Console] Lazy commands, separate IO concerns, add forced resolve to command, and prittify and detail output Aug 30, 2017
@robfrawley robfrawley mentioned this pull request Sep 6, 2017
Copy link
Contributor

@maximgubar maximgubar left a comment

Choose a reason for hiding this comment

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

also, one small notice, since php versions <= 5.5 are not maintained anymore, probably it would make sense to switch from array() to [] ?

*/
public function __construct(InputInterface $input, OutputInterface $output, bool $decoration = true)
{
$this->io = new SymfonyStyle($input, $output);
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt about moving instantiation of a SymfonyStyle to the factory and inject it in the constructor ?
cause new call breaks extensibility here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This whole thing is old and a newer version needs to be pushed here soon. Also, since this PR was originally submitted we already changed our code style to favor short array construction and destruction. See: https://github.com/liip/LiipImagineBundle/blob/2.0/.php_cs.dist

@maximgubar
Copy link
Contributor

since default branch was changed to master, please update the base branch for this PR

@dbu dbu changed the base branch from 2.0 to master August 9, 2019 08:46
@dbu
Copy link
Member

dbu commented Aug 22, 2019

@robfrawley i looked into a bunch of things on the bundle and hope to release a new version soon. do you plan to pick up this (and #993) again? both seem completely new features, so i see no problem releasing 2.2.0 without them.

@dbu
Copy link
Member

dbu commented Sep 10, 2019

continued in #1222

@dbu dbu closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Level: Enhancement ✨ This item involves an enhancement to existing functionality. State: Reviewing This item is being reviewed to determine if it should be accepted.
Projects
2.x Sprint 001
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants