search

helgatheviking / Nav-Menu-Roles

Display / Hide wp_nav_menu() items by role
65 stars 32 forks source link

First half of WPCS #45

Closed szepeviktor closed 4 years ago

szepeviktor commented 4 years ago

Run it

composer require --dev wp-coding-standards/wpcs dealerdirect/phpcodesniffer-composer-installer
vendor/bin/phpcs

inc/class.Walker_Nav_Menu_Edit_Roles_4.7.php and inc/class.Walker_Nav_Menu_Edit_Roles_4.5.php are not done yet.

Please share your feelings!

szepeviktor commented 4 years ago

Git does not really recognizes the two classes as changed! They have been fully WPCS-d :)

Here is a commit without filename change to review the changes https://github.com/szepeviktor/Nav-Menu-Roles/commit/9dc1a340e25e4b815727b83486a787a37a19cac3?w=1&diff=unified whitespace changes are hidden (?w=1)

helgatheviking commented 4 years ago

Thanks for this! I don't have a ton of time to review right now. I'm not thrilled about the mass indenting of everything as that clobbers the git history by touching the entire file. But I'm not sure there's a way around it either. In some places we could "quit early", but in some places that won't work either. Need to think about that...

szepeviktor commented 4 years ago

the mass indenting of everything

Do you mean following WPCS/WordPress-Core?

How to do that without changing indentations?

helgatheviking commented 4 years ago

Yes. You can sometimes do

if ( ! class_exists( 'some_class' ) ) {
    return;
}

class some_class {}

as a workaround. Learned that from the woman who writes most of the WP phpcs standards code. Just not applicable in all the cases here.

szepeviktor commented 4 years ago

Daily I work for SaaS teams and the basic set of tests are

  1. syntax check
  2. coding standards
  3. static analysis

I've started you in that direction! I hope you will finish this PR :)

szepeviktor commented 4 years ago

If you would like to start over, this is the command to run on the original files: vendor/bin/phpcbf providing the phpcs.xml file in this PR is in place.

helgatheviking commented 4 years ago

closed in favor of #48

szepeviktor commented 4 years ago

Okay.