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.

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

Separate phpcs.xml.dist Files for Each Plugin to Isolate Text Domains #1002

Merged
merged 11 commits into from
Feb 27, 2024
Merged

Separate phpcs.xml.dist Files for Each Plugin to Isolate Text Domains #1002

merged 11 commits into from
Feb 27, 2024

Conversation

mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Feb 21, 2024 •
edited
Loading

Summary

Fixes #997

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Feb 21, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Feb 21, 2024
thelovekesh
thelovekesh reviewed Feb 21, 2024
View reviewed changes
composer.json Show resolved Hide resolved
phpcs.ruleset.xml Show resolved Hide resolved
plugins/auto-sizes/.gitattributes Outdated Show resolved Hide resolved
plugins/auto-sizes/.gitignore Outdated Show resolved Hide resolved
@thelovekesh
Copy link
Member

Tests the PHPCS config with the following commands by changing the text domains, and it's working as expected:

Commands output 
➜  performance git:(fix/997-separate-phpcs) ✗ build-cs/vendor/bin/phpcs                                             
......E..................................................... 60 / 69 (87%)
.........                                                    69 / 69 (100%)



FILE: /home/thelovekesh/Desktop/loki/wordpressdev/public/content/plugins/performance/modules/database/audit-autoloaded-options/can-load.php
--------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------------
 15 | ERROR | Mismatched text domain. Expected 'performance-lab' or 'default' but got 'performance'. (WordPress.WP.I18n.TextDomainMismatch)
--------------------------------------------------------------------------------------------------------------------------------------------

Time: 2.3 secs; Memory: 36MB

➜  performance git:(fix/997-separate-phpcs) ✗ build-cs/vendor/bin/phpcs plugins/auto-sizes --standard=plugins/auto-sizes/phpcs.xml.dist
.E 2 / 2 (100%)



FILE: /home/thelovekesh/Desktop/loki/wordpressdev/public/content/plugins/performance/plugins/auto-sizes/hooks.php
---------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------
 29 | ERROR | Mismatched text domain. Expected 'auto-sizes' or 'default' but got 'auto-'. (WordPress.WP.I18n.TextDomainMismatch)
---------------------------------------------------------------------------------------------------------------------------------

Time: 85ms; Memory: 16MB

@swissspidy
Copy link
Member

Btw I am looking into those PHP unit test failures, probably has to do with yesterday's MariaDB update.

westonruter
westonruter reviewed Feb 21, 2024
View reviewed changes
plugins/auto-sizes/phpcs.xml.dist Outdated Show resolved Hide resolved
phpcs.xml.dist Show resolved Hide resolved
plugins/auto-sizes/phpcs.xml.dist Outdated Show resolved Hide resolved
joemcgill
joemcgill requested changes Feb 23, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

I like where this is headed, though I think we will need to update the various ways that we run PHPCS via local scripts to make sure we're running the correct ruleset for each plugin. I left a suggestion, but I'm not totally sold that this is the most maintainable approach.

phpcs.ruleset.xml Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
plugins/auto-sizes/phpcs.xml.dist Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
@westonruter westonruter mentioned this pull request Feb 23, 2024
Closed
This was referenced Feb 24, 2024
Merged
Closed
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review February 26, 2024 04:57
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 26, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter
westonruter approved these changes Feb 26, 2024
View reviewed changes
phpcs.xml.dist Outdated Show resolved Hide resolved
@westonruter westonruter mentioned this pull request Feb 27, 2024
Closed
felixarntz
felixarntz requested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 This looks great to me overall. A few small points of feedback.

phpcs.xml.dist Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
.gitattributes Show resolved Hide resolved
@mukeshpanchal27
Copy link
Member Author

@felixarntz @joemcgill I have addressed the feedback in the PR. It's ready for merge now. Could you please review it when you have a moment?

felixarntz
felixarntz approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @mukeshpanchal27, LGTM!

plugins/auto-sizes/phpcs.xml.dist

<rule ref="../../phpcs.ruleset.xml"/>

<config name="text_domain" value="auto-sizes"/>
Copy link
Member

Choose a reason for hiding this comment

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

Related to my previous feedback, we could add default as an allowed text domain for all plugins, but definitely not required as long as we don't use any core strings in them.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep this omitted unless we require core strings.

This was referenced Feb 27, 2024
Merged
Merged
@felixarntz
Copy link
Member

@joemcgill Can you please take another look at this as you previously requested changes?

joemcgill
joemcgill approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill 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 the updates. I think this looks good now.

@westonruter westonruter merged commit 3b40aa4 into feature/modules-to-plugins Feb 27, 2024
39 checks passed
@westonruter westonruter deleted the fix/997-separate-phpcs branch February 27, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@joemcgill joemcgill joemcgill approved these changes

@swissspidy swissspidy swissspidy approved these changes

@felixarntz felixarntz felixarntz approved these changes

@thelovekesh thelovekesh thelovekesh approved these changes

Assignees

@mukeshpanchal27 mukeshpanchal27

Labels
Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mukeshpanchal27 @thelovekesh @swissspidy @felixarntz @westonruter @joemcgill