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.

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

PrefixAllGlobals: add recording of metrics #1437

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 26, 2018

When a project which wasn't using WPCS, starts to use it or when an project upgrades to WPCS > 0.12.0, one of the things the project most likely will run into is the prefixing of all globals.

When no prefixes are passed, the sniff will not throw any errors.

However, if the project does want to comply and wants to add a prefix configuration, it would be useful to get some insight into which prefixes are currently in use in a project and how often each is used to determine what prefixes to allow and/or what prefixes to consolidate.

PHPCS offers a metrics mechanism which can be used for this, the results of which can be viewed using the --report-info argument.

This PR takes advantage of that metrics report to:

  1. Report on the frequency each of the allowed prefixes is used
  2. Report on potential additional prefixes and their usage frequency.

Example workflow and output when run against the current `trunk` of the bbPress plugin (fold out for the details)

When running WPCS against bbPress trunk initially with some runtime-set prefixes, 115 errors are reported:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
---------------------------------------------------------------------------------------
STANDARD  CATEGORY            SNIFF                                               COUNT
---------------------------------------------------------------------------------------
WordPres  Naming conventions  Prefix all globals non prefixed hookname found      48
WordPres  Naming conventions  Prefix all globals non prefixed constant found      28
WordPres  Naming conventions  Prefix all globals non prefixed class found         27
WordPres  Naming conventions  Prefix all globals non prefixed function found      8
WordPres  Naming conventions  Prefix all globals non prefixed variable found      4
---------------------------------------------------------------------------------------
A TOTAL OF 115 SNIFF VIOLATIONS WERE FOUND IN 5 SOURCES
---------------------------------------------------------------------------------------

Now, obviously you can use the full report to go through the details, but figuring out if there are persistent prefixes which have been overlooked can - with a large report - be a lot of work.

With the code in this PR, you could now run the following command:

phpcs -p ./src --standard=WordPress --sniffs=WordPress.NamingConventions.PrefixAllGlobals --report=info --runtime-set prefixes bbpress,bbp

... and see the exact break-down of the potential prefixes of these 115 violations:

PHP CODE SNIFFER INFORMATION REPORT
----------------------------------------------------------------------
Allowed prefix found: bbp [2863/2867, 99.86%]
        bbpress => 4 (0.14%)

Potential prefixes: found at start of non-prefixed construct: bbcode [30/114, 26.32%]
        filter => 16 (14.04%)
        bbp => 7 (6.14%)
        the => 4 (3.51%)
        bp => 4 (3.51%)
        login => 3 (2.63%)
        editable => 3 (2.63%)
        wp => 2 (1.75%)
        edit => 2 (1.75%)
        is => 2 (1.75%)
        comment => 2 (1.75%)
        v => 2 (1.75%)
        loop => 2 (1.75%)
        phorum => 1 (0.88%)
        php => 1 (0.88%)
        phpfox3 => 1 (0.88%)
        mingle => 1 (0.88%)
        phpwind => 1 (0.88%)
        my => 1 (0.88%)
        kunena3 => 1 (0.88%)
        kunena2 => 1 (0.88%)
        kunena1 => 1 (0.88%)
        invision => 1 (0.88%)
        flux => 1 (0.88%)
        example => 1 (0.88%)
        e107v1 => 1 (0.88%)
        drupal7 => 1 (0.88%)
        aef => 1 (0.88%)
        pun => 1 (0.88%)
        simple => 1 (0.88%)
        register => 1 (0.88%)
        vanilla => 1 (0.88%)
        show => 1 (0.88%)
        user => 1 (0.88%)
        desc => 1 (0.88%)
        name => 1 (0.88%)
        stats => 1 (0.88%)
        enable => 1 (0.88%)
        smf => 1 (0.88%)
        action => 1 (0.88%)
        buddypress => 1 (0.88%)
        key => 1 (0.88%)
        doing => 1 (0.88%)
        after => 1 (0.88%)
        xmb => 1 (0.88%)
        xen => 1 (0.88%)
        edited => 1 (0.88%)
        plugin => 1 (0.88%)

----------------------------------------------------------------------

N.B.: Yes, I know, there is one missing - if the function name would be __ and in some other exceptional circumstances, no valid potential prefix can be determined, which is why the total can be slightly off.


I imagine that if this PR is accepted, it would be useful to add a wiki page about how to get access to this information and how to use it.

Notes about the implementation:

  • For the "potential prefixes" a relatively simple formula is used:
    • Strip off any leading underscores, namespace separators and variable $ signs from the name as found.
    • Grab the first part of the name, before the first underscore or next uppercase character.
      This matches Pascal in PascalCase, camel in camelCase, snake in snake_case.
  • Potential prefixes are passed to the metrics mechanism in lowercase as (nearly all) the prefix checks are done in a case-insensitive manner.
  • Potential prefixes are only recorded for those instances where an error/warning would be reported by the sniff. If a whitelist comment, a PHPCS ignore annotation or a ruleset exclude-pattern, prevents the error from being recorded, no "potential prefix" metric will be recorded.
  • The PR changes the method signature of two methods in the sniff, but as both are private methods, this is not a BC-break.
  • Add a wiki page about how to go about prefixing all globals in legacy projects.

@jrfnl jrfnl added this to the 1.0.1 milestone Jul 26, 2018
When a project which wasn't using WPCS, starts to use it or when an project upgrades to WPCS > 0.12.0, one of the things the project most likely will run into is the prefixing of all globals.

When no prefixes are passed, the sniff will not throw any errors.

However, if the project does want to comply and wants to add a prefix configuration, it would be useful to get some insight into which prefixes are currently in use in a project and how often each is used to determine what prefixes to allow and/or what prefixes to consolidate.

PHPCS offers a metrics mechanism which can be used for this, the results of which can be viewed using the `--report-info` argument.

This PR takes advantage of that metrics report to:
1. Report on the frequency each of the allowed prefixes is used
2. Report on potential additional prefixes.

I imagine that if this PR is accepted, it would be useful to add a wiki page about how to get access to this information and how to use it.

Notes about the implementation:
* For the "potential prefixes" a relatively simple formula is used:
    - Strip off any leading underscores, namespace separators and variable `$` signs of the name as found
    - Grab the first part of the name, before the first underscore or next uppercase character.
        This matches `Pascal` in `PascalCase`, `camel` in `camelCase`, `snake` in `snake_case`.
* Potential prefixes are passed to the metrics mechanism in lowercase as (nearly all) the prefix checks are done in a case-insensitive manner.
* Potential prefixes are only recorded for those instances where an error/warning would be reported by the sniff. If a whitelist comment, a PHPCS ignore annotation or a ruleset `exclude-pattern`, prevents the error from being recorded, no "potential prefix" metric will be recorded.
* The PR changes the method signature of two methods in the sniff, but as both are `private` methods, this is not a BC-break.

- [ ] Add a wiki page about how to go about prefixing all globals in legacy projects.
@jrfnl jrfnl force-pushed the feature/prefix-all-globals-add-metrics branch from fec372d to f63c60f Compare July 27, 2018 01:06
@jrfnl
Copy link
Member Author

jrfnl commented Aug 15, 2018

@WordPress-Coding-Standards/wpcs-admins Anyone has an opinion on this PR ?

Copy link
Contributor

@JDGrimes JDGrimes left a comment

Choose a reason for hiding this comment

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

Looks useful, 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants