-
Notifications
You must be signed in to change notification settings - Fork 77
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
[garbage-collector] Initial commit. #649
Conversation
…s also covers add-on–related use cases; properly store updated options and store them only once).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fajardoleo Thanks for the PR. I think I understand the logic, but I do believe the code can be made more organized. Please see my suggestion, thanks 👍
…ating large method(s) into multiple smaller methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fajardoleo Great job on the refactoring 👏 . Please see my few suggestions, I think we can simplify it/make it more readable. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fajardoleo Nice refactoring. You have managed to get rid of several if..else
in the logic and the code is much more readable now. Please see my suggestion for some easy improvements. Thanks.
} | ||
|
||
if ( ! $updated ) { | ||
if ( isset( $option[ $slug ] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question @fajardoleo : Are all these checks mutually exclusive? So when $option[ $slug ]
is present we won't need to check for $option[ "{$slug}:{$this->get_type()}" ]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swashata That is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fajardoleo looks like I get it now. If we ever go with my improvement suggestion (one GC class per option) we can get rid of this, but not important right now at the moment. I have added a @todo
.
5be4d83
to
b2c48da
Compare
…age collect. 1. Consider {product}_data when doing garbage collection. 2. Don't garbage collect all_{module} and active_{module} since those have different structure and source. Add @todo to implement them later. 3. Properly handle missing timestamp in the garbage collector, adding zero risk to the existing BL. 4. Disable garbage collection by default and add it behind a flag => WP_FS__ENABLE_GARBAGE_COLLECTOR. 5. Simplify some logic where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fajardoleo I implemented my suggestions and fixed the typos I found. Now please review my commit and test :D
} | ||
|
||
if ( ! $updated ) { | ||
if ( isset( $option[ $slug ] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fajardoleo looks like I get it now. If we ever go with my improvement suggestion (one GC class per option) we can get rid of this, but not important right now at the moment. I have added a @todo
.
…erridden during operation and how the gc timestamp is stored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.