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

Fix codestyle. #590

Merged
merged 8 commits into from
Jul 14, 2016
Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 12, 2016

Complies with WP-Core.
As there are only two warnings left when run against WP-Extra, I've activated that as well. Might as well lead by example ;-)

Still would need some work to comply with WP-Docs. When run against Docs PHPCS currently yields some 100 or so issues mostly to do with file vs class docblocks.

Also:

  • Removed some large commented out code blocks.
  • Minor spelling and punctuation corrections.
  • Few minor fixes to do with variable comparisons, mainly changing loose to strict comparisons.
  • Applied some best practices from the Docs and Extra standards.
  • Variable and array value alignment when I saw it.

Closes #256

'wp_kses' => true,
'wp_kses_data' => true,
'wp_kses_post' => true,
'wp_kses' => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I'm not big on these array alignment. It can make things look better, but IMO it isn't necessary when all of the values are currently the same, and it will inevitably get out of sync next time a longer function is added to the list. When that happens we're either going to realign the whole array, or we'll end up with a partly aligned array, which after a while will put us back in square one. :-) I'm not going to complain if others disagree, but I just thought I'd mention it in case anybody wants to give a second opinion.

For the record, the variable alignments look good, I'm just talking about these array alignments.

Or do we still sniff for array alignments? I don't think that we do. Or else maybe I've disabled that (but I don't think so)...

Copy link
Member Author

Choose a reason for hiding this comment

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

Or do we still sniff for array alignments?

No, we don't.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer alignment for the multiple times code like this will be read by developers, over the relative few instances that someone might need to adjust the spacing because of an addition that has more characters. FWIW, in PHPStorm you can highlight, hit a keystroke (mine is CMD+Shift+L), and they will all align anyway - no need for manual alignment.

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 it would complicate things if we say we don't do variable alignments if the value is the same otherwise we we do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, we can keep them aligned. My issue isn't that it will be trouble to realign them (I use PHPStorm as well), but that doing so makes history harder to follow. But I guess it does make sense to prioritize the most common use, which is devs looking at the lists just as they are.

As this clashes with `WordPress.WhiteSpace.ControlStructureSpacing.BlankLineAfterEnd` for the last method in a class, I've - for now - resorted to adding `// end` comments after the method closing brace to bypass that error.
jrfnl added 2 commits July 13, 2016 06:02
* Fix some missed "class opening brackets on different line" issues. There currently is no sniff checking for this, so visual fix.
* Fixed one class extending the wrong parent.
* Added generic file docblock to sniff files which didn't have one.
* Added/replaced generic file docblock for all unit test files.
* Use `@link` rather than `@see` for links to external documentation about a sniff in the file/class docblock.
* Consistent placement of the `@link` tag if available and pointing to WPCS relevant info.
* Removed `PHP version 5` notes. No longer relevant.
* Fixed a few class description referring to the wrong rule/sniff.

Docs still need a lot of love to get up to scratch properly.
@jrfnl
Copy link
Member Author

jrfnl commented Jul 13, 2016

Added two more commits:

Minor touch-up.

  • Fix some missed "class opening brackets on different line" issues. There currently is no sniff checking for this, so visual fix.
  • Fixed one class extending the wrong parent.

Documentation fixes.

  • Added generic file docblock to sniff files which didn't have one.
  • Added/replaced generic file docblock to all unit test files.
  • Use @link rather than @see for links to external documentation about a sniff in the file/class docblock.
  • Consistent placement of the @link tag if available and pointing to WPCS relevant info.
  • Removed PHP version 5 notes. No longer relevant.

Docs still need a lot of love to get up to scratch properly. This is just a small step in the right direction.

@JDGrimes JDGrimes added this to the 0.10.0 milestone Jul 13, 2016
@JDGrimes
Copy link
Contributor

As far as I am concerned, this is ready for merge. Any other comments or objections?

@GaryJones
Copy link
Member

:shipit:

@JDGrimes JDGrimes merged commit 77e400c into WordPress:develop Jul 14, 2016
@jrfnl jrfnl deleted the WPCS/feature/wpcs-codestyle branch July 14, 2016 16:41
jrfnl added a commit to jrfnl/WordPress-Coding-Standards that referenced this pull request Aug 1, 2016
jrfnl added a commit to jrfnl/WordPress-Coding-Standards that referenced this pull request Aug 18, 2016
jrfnl added a commit to jrfnl/WordPress-Coding-Standards that referenced this pull request Aug 26, 2016
jrfnl added a commit to jrfnl/WordPress-Coding-Standards that referenced this pull request Sep 11, 2016
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.

Settle on coding convention for WPCS snfifs
4 participants