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 various PHPCS issues #1002

Merged
merged 19 commits into from Mar 17, 2018

Conversation

Projects
None yet
2 participants
@paulschreiber
Copy link
Contributor

commented Mar 8, 2018

No description provided.

@westonruter

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

@paulschreiber if you re-base you recent pull requests against develop the build errors related to PHP 5.3 should go away.

Also, feel free to combine your commits into a single PR that fixes multiple phpcs issues in one go. No need for separate PRs for each commit.

@paulschreiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2018

  • I created separate PRs to make reviewing the diffs easier. I also wasn't sure which ones you would accept.
  • This was branched off develop. What sort of rebasing is needed?
@westonruter

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

I created separate PRs to make reviewing the diffs easier. I also wasn't sure which ones you would accept.

Normally that's good, but these PRs are almost all PHPCS fixes which can be easily reviewed together. Having a separate PR for each just creates too much noise.

This was branched off develop. What sort of rebasing is needed?

A fix just landed in develop which fixes the PHP 5.3 issue: a04782a

So just git pull --rebase https://github.com/Automattic/amp-wp.git develop; git push -f and that should fix the build.

@westonruter

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

Please then feel free to git cherry-pick each commit from your other PHPCS-related pull requests to amend to this PR, and then close the other PRs.

@paulschreiber paulschreiber force-pushed the paulschreiber:fix/admin-bar branch from a5eeba0 to 65fcf5a Mar 10, 2018

@paulschreiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

I've rebased and cherry-picked the commits (except for #1000, as it's not clear what do there).

@paulschreiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

@westonruter What's causing Travis CI to fail here?

@westonruter

This comment has been minimized.

Copy link
Member

commented Mar 10, 2018

@paulschreiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

This commit (be6a78a) doesn't touch class-amp-base-embed-handler.php, which generates the error.

@westonruter

This comment has been minimized.

Copy link
Member

commented Mar 10, 2018

@paulschreiber there are multiple errors: https://travis-ci.org/Automattic/amp-wp/jobs/351608818#L315-L337

PHPCS via wp-dev-lib is looking at changes made to the entire diff in the PR, not individual commits in the PR. Because you added public to a method that lacks the necessary phpdoc, now that PHPCS error is being surfaced because it is related to a line you touched.

@paulschreiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

Odd. We saw a green checkmark after the previous commit (df77083). Do you want me to revert 78e64f4 or add docs?

@westonruter

This comment has been minimized.

Copy link
Member

commented Mar 10, 2018

Adding the doc comments would be great.

@paulschreiber

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

I've added many doc comments. PHPCS is still finding a few errors:

class-amp-base-embed-handler.php:59:32: error - Object property "DEFAULT_WIDTH" is not in valid snake_case format
class-amp-youtube-embed.php:161:13: warning - This comment is 50% valid code; is this commented out code?
class-amp-youtube-embed.php:170:13: warning - This comment is 47% valid code; is this commented out code?
test-amp-image-dimension-extractor.php:66:1: warning - Best practice suggestion: Declare only one class in a file.
test-amp-image-dimension-extractor.php:130:1: warning - Best practice suggestion: Declare only one class in a file.

Should we leave those for now?

@@ -229,7 +229,7 @@ public static function add_hooks() {
* Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone
* combine to surpass the 50K limit imposed for the amp-custom style.
*/
add_filter( 'show_admin_bar', '__return_false', 100 );
add_filter( 'show_admin_bar', '__return_false', 100 ); // phpcs:ignore

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 17, 2018

Member

This actually isn't necessary. The PHPCS ruleset does not include the AdminBarRemoval WordPress.com VIP sniff. You should make sure to use the project's own ruleset when developing.

@westonruter westonruter changed the title suppress PHPCS warning for show_admin_bar Fix various PHPCS issues Mar 17, 2018

array(
'url' => $url,
'video_id' => $video_id,
)

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 17, 2018

Member

Aside: things like this are more elegant as compact( 'url', 'video_id' )

This comment has been minimized.

Copy link
@paulschreiber

paulschreiber Mar 18, 2018

Author Contributor

Agreed that compact is better. phpcbf reformatted it as above (expanded).

@westonruter westonruter added this to the v1.0 milestone Mar 17, 2018

@westonruter westonruter merged commit 79526a4 into ampproject:develop Mar 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

westonruter added a commit that referenced this pull request May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.