Update 4 letters for min prefix length #2479
Conversation
There was a problem hiding this comment.
Thanks for PR @davidperezgar.
Can you please do things properly though ? It looks like you don't understand how docs work.
- The
@sincetag should NOT be changed as it indicated when the constant was added. A second@sincetag can be added to in explain that the value of the constant changed in WPCS 3.2.0. - The
@linktag should NOT be changed as it is a reference to explain why the constant was added. A secondary@linktag can be added to the new ticket to explain why it was raised.
Also, this is a functional change, so the tests will need to be updated to match, as can also be seen by the failing build.
|
Ha, looks like @dingo-d and me were reviewing at the same time, but had the same feedback ;-) |
|
Ok! I'll check it , thanks. |
|
I don't understand how it works the unit testing in the sniff.. I'd need some help... |
Did you read the CONTRIBUTING guide ? |
WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc
Outdated
Show resolved
Hide resolved
Yes! |
WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc
Outdated
Show resolved
Hide resolved
|
I see that this sniff is included in WordPress-Extra. I also see the history of this change is related to plugins, but what about themes and the theme review team? This change will affect those folks who are writing themes with custom functions too - has that impact been considered? |
|
Hello, yes it will impact to themes as well, but we have been considered the benefits of this measure, and that's why is now in the Developers HandBook: We were using this number of prefixes for a some time ago and we are unifying everywhere. I'm going to ping Themes Team to be aware of it. |
|
There are several mentions of "prefix" in the theme developers handbook, but:
... seem to be the missed opportunities to clarify/define the specific minimum length of such prefixes. It seems right (to me) to get their buy-in on this before we merge this PR; the Plugins Review Team can continue to require 4 characters as they have apparently have for a while anyway). |
|
I've spoken with Team Rep and they will check with the Team. I don't see a problem for that. Thanks @GaryJones |
|
@GaryJones Thank you for sharing the link. Once we decided to have 4 letter for min prefix length, we'll discuss within the team for updating the handbook. |
|
Thanks! @kafleg ! |
davidperezgar
force-pushed
the
develop
branch
from
3e6d477 to
c94f358
Compare
|
I believe now is ready to be reviewed. Thanks. |
There was a problem hiding this comment.
Thanks for implementing the changes, @davidperezgar! Looks good to me now.
@WordPress/wpcs, as far as I can check, all the points raised were addressed and this PR is ready to be merged.
Edit: @jrfnl (as we discussed this via DM), it seems that mentioning teams is not working in this repository. Maybe due to the way permissions are configured in this GH organization. Mentioning the other maintainers as well directly as the team mention above didn't work: @GaryJones and @dingo-d.
|
@davidperezgar and @kafleg, any updates on including the minimum four-letter prefix rule in the theme handbook? @GaryJones mentioned he'd prefer this information to be added to the theme handbook before merging this PR. |
|
Updated the guidelines doc, https://make.wordpress.org/themes/handbook/review/required/ mentioning in the prefix section. |
Fixes #2467