Rename Site Health check modules for language and consistency #423
Conversation
mxbclang
added
Infrastructure
OllieJones
requested review from
manuelRod,
westonruter,
adamsilverstein,
kirtangajjar and
mitogh
as code owners
mukeshpanchal27
added
the
no milestone
|
IMO plugin creates a folder structure based on the module name, so if we change the module name, then it does not match. |
mukeshpanchal27
removed
the
no milestone
|
@felixarntz Tagging you in to take a quick look at this one based on @mukeshpanchal27's comment above. |
There was a problem hiding this comment.
Thanks @OllieJones! Potentially we should rename the module slugs (i.e. move the directories) as well?
There was a problem hiding this comment.
After further thought, I tend to agree with @mukeshpanchal27's feedback in #423 (comment) that we should probably rename the modules as a whole.
@OllieJones Could you rename the module directories and references to it?
This is not extremely critical and the deadline for release PRs is today, so I'm going to remove this one for now, we can target it for 1.4.0.
Backmerge latest changes.
|
@OllieJones We are two weeks away from the |
|
I will do this. I need clarification. I think you want, for example, the Autoloaded Options Health Check module to be placed in the directory |
Backmerge
|
It should be |
…ull-page-cache to full-page-cache-health-check. Change directory and file names under tests to match.
|
I made the changes requested by @mukeshpanchal27 and @felixarntz . |
There was a problem hiding this comment.
@OllieJones Apologies, I am now seeing another problem here regarding the module renaming (even though I originally proposed it): Renaming the modules is problematic since it may already be stored in the sites' DB options, or developers may use the module slugs in the filter. So it would result in breakage currently where if a site had one of the remained modules active, it would no longer be active after the update, which we must avoid.
If we want to go through with the module slug/directory changes, at a minimum we would need to add logic to retrieving the option value so that it migrates the old values to the new ones. But even that would not be 100% proof.
So I think (sorry again about that) it would be best to go back to your previous implementation where you only adjust the module names themselves. One thing missing here then is to also update the CODEOWNERS file comments where these same names are referenced accordingly. Also see the related #465 PR - we need to decide which one of these naming conventions to go with. cc @bethanylang
|
Thanks @felixarntz! My personal preference would be to go with @OllieJones' approach and add "Health Check" to the end of each appropriate module title in order to distinguish those from the other, non-Health Check modules, but let me know what you think. |
|
@bethanylang Thanks, SGTM. @OllieJones Can you revert the directory name changes in this PR so that it's back to only changes to the actual module titles? I'll move this out of the 1.4.0 milestone for now since it's not ready yet, but if it gets ready tomorrow, we could potentially still include it in this release. |
… audit-full-page-cache to full-page-cache-health-check. Change directory and file names under tests to match." This reverts commit 8cf5013. Undo module slug name changes per Felix Arntz's request at #423 (comment)
|
The module slugs (directory names) are now reverted. Should be ready to merge. |
|
Thanks, @OllieJones! There are still changes that need to be made per @felixarntz's requests here: https://github.com/WordPress/performance/pull/423/files/8cf50133dacc686c153dfb632bfb0feb6369a497. Can you make those updates when you get a moment, as well? Then this should be ready to merge and I can update #465. Thanks! |
|
@bethanylang I think actually everything has been addressed now. Since we're only changing the module names here vs the slugs, no migration of the option should happen. I think we're good to merge this and then update #465. |
felixarntz
changed the title
Summary
Updated the (English) language captions in the Settings panel.
Changed the site-health slugs to, for example, to
autoloaded-options-health-checktoaudit-autoloaded-options.Made corresponding changes in the unit-test directory and file names.
(This very easy dev task will help me ensure I know how to drive the PR workflow.)
Fixes #326
Relevant technical choices
Trivial editing of the strucutured comments at the top of four load.php files
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.