8000 [FrameworkBundle] Add `--exclude` option to the `cache:pool:clear` command by MatTheCat · Pull Request #51058 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Add --exclude option to the cache:pool:clear command #51058

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
Oct 11, 2023
Merged

[FrameworkBundle] Add --exclude option to the cache:pool:clear command #51058

merged 1 commit into from
Oct 11, 2023

Conversation

MatTheCat
Copy link
Contributor
@MatTheCat MatTheCat commented Jul 21, 2023 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #51023
License MIT
Doc PR symfony/symfony-docs#18595

For now this PR just ignores excluded pools/clearers when they don’t exist or wouldn’t be cleared/run anyways. Not sure what the best DX would be 🤔

@carsonbot carsonbot added this to the 6.4 milestone Jul 21, 2023
@MatTheCat MatTheCat force-pushed the exclude_pools_from_clearing branch from a9a13cc to 5126d11 Compare July 21, 2023 09:52
maxbeckers
maxbeckers approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor
@maxbeckers maxbeckers left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could also be done by not adding it to $pools and $clearers in the loop before instead of this unset solution, but fine for me this way.

Needs a related doc PR to explain the feature in docs.

@MatTheCat MatTheCat mentioned this pull request Jul 21, 2023
Merged
@MatTheCat MatTheCat force-pushed the exclude_pools_from_clearing branch from 5126d11 to 39fe666 Compare July 21, 2023 10:20
@nicolas-grekas
Copy link
Member

Please always rebase, we don't accept PRs with merge commits in their history.

@MatTheCat MatTheCat force-pushed the exclude_pools_from_clearing branch from b3312ea to fa93e0c Compare August 8, 2023 15:24
@MatTheCat
Copy link
Contributor Author

Sorry, tried to rebase using GitHub UI but it ended in a merge commit 😅
Should be good now.

@chalasr
Copy link
Member
chalasr commented Aug 11, 2023 •
edited
Loading

We could deprecate --all and just clear all pools when the pools argument is not specified.

@OskarStark
Copy link
Contributor

We could deprecate --all and just clear all pools when the pools argument is not specified.

To me this could be done in a separate PR, while this has some implicit drawbacks IMHO, because you could clear more than expected 👎

I would go with this PR as is

@MatTheCat MatTheCat force-pushed the exclude_pools_from_clearing branch from fa93e0c to 21d0348 Compare October 10, 2023 14:30
@OskarStark
Copy link
Contributor

One last proposal from a DX perspective could be to give autocompletion for the possible values?

@fabpot
Copy link
Member
fabpot commented Oct 11, 2023

Thank you @MatTheCat.

@fabpot fabpot merged commit 2101962 into symfony:6.4 Oct 11, 2023
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Oct 11, 2023
@OskarStark
feature #18595 [Cache] Document cache:pool:clear’s new --exclude …
f505c64 
…option (MatTheCat)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Cache] Document `cache:pool:clear`’s new `--exclude` option

Feature PR
* symfony/symfony#51058

Commits
-------

cc7ac50 [Cache] Document `cache:pool:clear`’s new `--exclude` option
@MatTheCat MatTheCat deleted the exclude_pools_from_clearing branch October 11, 2023 07:47
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@maxbeckers maxbeckers maxbeckers approved these changes

Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
7 participants
@MatTheCat @nicolas-grekas @chalasr @OskarStark @fabpot @maxbeckers @carsonbot
0