PrefixAllGlobals: verify that namespace names use a prefix #1515
Conversation
Until now, namespaced classes, functions and constants would be exempt from the "prefix all globals" rule, but the namespace name _itself_ was not examined.
As namespace names can, of course, also conflict, the namespace name should be prefixed with a plugin/theme specific prefix.
This PR adds that feature to the sniff. The PR builds onto the changes made in PR 1491 and 1492.
Notes:
* The sniff allows for underscores and (other) non-word characters in a passed prefix to be converted to namespace separators when used in a namespace name.
In other words, if a prefix of `my_plugin` is passed as the value of the custom property, a namespace name of `My\Plugin` will be accepted automatically.
* Passing a prefix property value containing namespace separators will now also be allowed and will no longer trigger a warning.
jrfnl
force-pushed
the
feature/prefixallglobals-namespace-prefixes
branch
from
ee3f771 to
f6da0aa
Compare
| @@ -0,0 +1,30 @@ | |||
| <?php | |||
| // @codingStandardsChangeSetting WordPress.NamingConventions.PrefixAllGlobals prefixes acronym,tgmpa,test_this,test\that | |||
|
|
|||
There was a problem hiding this comment.
One thing I've found beneficial when working on VIPCS, was to add a comment for each test case to identify what case was being tested. e.g.
namespace TGMPA; // Ok - case-insensitive namespace.
namespace Acronym\B\C; // Ok - prefix is top-level namespace, with sub-namespaces.
namespace AcronymPlugin\B\C; // Ok - prefix is start of top-level namespace, with sub-namespaces.
namespace Test_This\C; // Ok - namespace with underscore preserved from prefix.
namespace Test\This\C; // Ok - namespace with underscore converted to sub-namespace from prefix.
namespace Test\That\C; // Ok - prefix is multi-level namespace.
etc. Same goes for // Bad cases - adding a comment as to what aspect of the logic something fails.
I think this makes it easier to follow and understand the test cases, and helps avoid redundant test cases that don't test anything new.
Could comments like that be added here?
There was a problem hiding this comment.
Go right ahead if you feel the need. To me, those comments don't add any value as this sniff only has one purpose which is checking if something is prefixed.
There was a problem hiding this comment.
But the different test cases are checking all the different ways that it can be prefixed - the how, rather than the what.
I'm not going to be able to get these comments in the near future. The review is already approved. Probably needs a new ticket to add comments in for OK and Bad tests cases across all the sniffs.
There was a problem hiding this comment.
But the different test cases are checking all the different ways that it can be prefixed - the how, rather than the what.
That is documented in both the sniff as well as the commit message.
There are definitely some cases where additional comments in the unit test files could have value, but let's not go overboard.
Probably needs a new ticket to add comments in for OK and Bad tests cases across all the sniffs.
Sounds like a plan, low priority though as there are plenty of real issues to address still. Possibly mark it as good first issue ?
The review is already approved.
As we changed to one approval, feel free to merge it.
Until now, namespaced classes, functions and constants would be exempt from the "prefix all globals" rule, but the namespace name itself was not examined.
As namespace names can, of course, also conflict, the namespace name should be prefixed with a plugin/theme specific prefix.
This PR adds that feature to the sniff. The PR builds onto the changes made in PR #1491 and #1492.
Notes:
In other words, if a prefix of
my_pluginis passed as the value of the custom property, a namespace name ofMy\Pluginwill be accepted automatically.