Skip to content

Remove js from phpcs check#71

Merged
claudiulodro merged 1 commit into
masterfrom
update/phpcsjs
May 22, 2019
Merged

Remove js from phpcs check#71
claudiulodro merged 1 commit into
masterfrom
update/phpcsjs

Conversation

@claudiulodro
Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

Related #64

  • Removes the JS check from PHPCS.
  • We have prettier working nicely, and having two linters for JS code style seems excessive. Keeping rules for the two linters in sync would be difficult (so the lint rules don't conflict) .
  • PHPCS is not designed to handle React. The linter cannot handle code like <div foo={ bar }> and our codebase generates hundreds of lines of errors similar to the following:
FILE:newspack/wp-content/plugins/newspack-plugin/assets/src/wizards/componentsDemo/index.js
------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 330 ERRORS AND 75 WARNINGS AFFECTING 129 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------------
  47 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 12 spaces but found 1 space
     |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
  53 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 12 spaces but found 1 space
     |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
  72 | ERROR   | [x] Expected 1 space after "<"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
  72 | ERROR   | [x] Expected 1 space before ">"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
  73 | ERROR   | [x] Expected 1 space after "<"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
  74 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 19 spaces but found 0 spaces
     |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
  74 | ERROR   | [x] Expected 1 space before "="; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
  74 | ERROR   | [x] Expected 1 space after "="; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
  75 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 16 spaces but found 0 spaces
     |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
  75 | ERROR   | [x] Expected 1 space before "="; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
  75 | ERROR   | [x] Expected 1 space after "="; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
  76 | ERROR   | [x] Expected 1 space after "/"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceAfter)
  76 | ERROR   | [x] Expected 1 space before ">"; 0 found (WordPress.WhiteSpace.OperatorSpacing.NoSpaceBefore)
...

How to test the changes in this Pull Request:

  1. composer install if phpcs isn't already installed.
  2. vendor/bin/phpcs. You should just have one warning about @package tag in the main plugin file.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@claudiulodro claudiulodro added the [Status] Needs Review The issue or pull request needs to be reviewed label May 21, 2019
@jeffersonrabb jeffersonrabb self-requested a review May 21, 2019 21:56
Copy link
Copy Markdown
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

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

Looks good.

@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels May 21, 2019
@claudiulodro claudiulodro merged commit ab82692 into master May 22, 2019
@claudiulodro claudiulodro deleted the update/phpcsjs branch May 22, 2019 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants