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 prettify and detail output #1222

Merged
merged 2 commits into from
Sep 18, 2019

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Sep 9, 2019

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

As some of the previous PRs, this is to pick #981 up, there was a lot of work done and it's a pity to leave it behind.

@franmomu franmomu changed the title [Console] Lazy commands, separate IO concerns, add forced resolve to command, and prittify and detail output [Console] Lazy commands, separate IO concerns, add forced resolve to command, and prettify and detail output Sep 9, 2019
Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks for reviving this one. i commented on a couple of places.

Tests/Fixtures/BufferedOutput.php Show resolved Hide resolved
Command/CacheCommandTrait.php Outdated Show resolved Hide resolved
Command/RemoveCacheCommand.php Outdated Show resolved Hide resolved
Command/RemoveCacheCommand.php Outdated Show resolved Hide resolved
Command/ResolveCacheCommand.php Outdated Show resolved Hide resolved
Command/ResolveCacheCommand.php Outdated Show resolved Hide resolved
Command/RemoveCacheCommand.php Show resolved Hide resolved
Command/ResolveCacheCommand.php Outdated Show resolved Hide resolved
Component/Console/Style/ImagineStyleInterface.php Outdated Show resolved Hide resolved
- 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"
@franmomu franmomu force-pushed the feature-lazy-rewritten-commands branch from 3ac7b23 to 3a3c6af Compare September 10, 2019 21:45
@franmomu franmomu force-pushed the feature-lazy-rewritten-commands branch from 3eb2a69 to 36039f2 Compare September 16, 2019 23:00
Copy link
Member

@dbu dbu 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, thanks!

@dbu dbu merged commit a0ad3ba into liip:master Sep 18, 2019
@franmomu franmomu deleted the feature-lazy-rewritten-commands branch September 18, 2019 06:35
@robfrawley
Copy link
Collaborator

@franmomu: After reviewing the many PRs you've resurrected in the past few months (all of which were in a state of extreme merge-conflict with the current git HEAD, to the point were no one else, even their original contributors like myself, had any interest/time/patience in resolving), I felt entirely compelled to take a moment and extend my sincere gratitude for the excellent work you put in to get #1220, #1219, #1201, and #1199 successfully merged. I honestly resigned myself to the fact that the respective original PRs would never be introduced into master.

The patience, effort, and man-hours required to merge these PRs deserves praise and I wanted to ensure that I took the time to do so (it's the very least I could do :-). Many of the previously stalled PRs that you resurrected include significant new features, bug fixes, and structural refactoring that the entire LiipImagineBundle community will benefit from.

Really great work. Thanks so much for your contributions to LiipImagineBundle!

@franmomu
Copy link
Contributor Author

franmomu commented Oct 4, 2019

Thank you very much for your kind words! You made my day :D Those PR had a lot of good work done and it was a pity to let them rot and also it was a way to learn :)

@dbu
Copy link
Member

dbu commented Oct 7, 2019

I can only second what @robfrawley is saying, thanks a TON for all the effort you are putting into this @franmomu ! And thanks a lot @robfrawley for all the work you put into this bundle into the past! And thanks my co-worker at Liip @michellesanver who stepped up to maintain this bundle and waded through all those old issues last week! 💚

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.

None yet

3 participants