Add new line at the end of file and missing white space

[truongwp]

This PR add a missing white space between css selector and open brace, and add new line at the end of many files.

[szepeviktor]

Maybe adding a PHPCS rule for that?

[jrfnl]

Maybe adding a PHPCS rule for that?

There already is one included ;-) The reason these files were not reported is that PHPCS is currently configured in phpcs.xml.dist to only check PHP files, not JS or CSS files.

[grappler]

I would like to see this being checked for in travis before the changes are made so that they do not get reversed by mistake in the future.

[truongwp]

Hi @grappler,
How I can do that?

[grappler]

Add this to .travis.yml would check the CSS and JS files.
- if [[ "$SNIFF" == "1" ]]; then $PHPCS_DIR/scripts/phpcs . --sniffs=Generic.Files.EndFileNewline --extensions=css,js; fi

The problem is that PHPCS would not not check the other files like scss.

We need to come up with another solution that could run in travis to check this.

[grappler]

Here is another way we could run this in bash https://stackoverflow.com/a/34944104/850651

[jrfnl]

The problem is that PHPCS would not not check the other files like scss.

You can pass scss as an additional extension to PHPCS to be checked as if it were CSS. --extensions=css,scss/css,js should do the trick.

You could also consider changing the extensions directive in the custom ruleset. There are not that many sniffs in WPCS which check CSS and JS any way, so - except for minified files - I don't really expect any problems.

[grappler]

--extensions=css,scss/css,js does do the trick.

The only thing that is not checked are the "hidden" files .jshintignore & .jscsrc but they are not worked on a lot and they maybe replaced by package.json in the future.

If we enable PHPCS for JS then will need to fix the following warnings.

FILE: _s/js/customizer.js
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | ERROR | Missing @package tag in file comment
   |       | (Squiz.Commenting.FileComment.MissingPackageTag)
----------------------------------------------------------------------

FILE: _s/js/navigation.js
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
   6 | ERROR | There must be exactly one blank line after the file
     |       | comment (Squiz.Commenting.FileComment.SpacingAfterComment)
   6 | ERROR | Missing @package tag in file comment
     |       | (Squiz.Commenting.FileComment.MissingPackageTag)
  89 | ERROR | The use of object.length inside a loop condition is not
     |       | allowed; assign the return value to a variable and use the
     |       | variable in the loop condition instead
     |       | (Squiz.PHP.DisallowSizeFunctionsInLoops.Found)
 101 | ERROR | The use of object.length inside a loop condition is not
     |       | allowed; assign the return value to a variable and use the
     |       | variable in the loop condition instead
     |       | (Squiz.PHP.DisallowSizeFunctionsInLoops.Found)
----------------------------------------------------------------------

FILE: _s/js/skip-link-focus-fix.js
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | ERROR | There must be exactly one blank line after the file
   |       | comment (Squiz.Commenting.FileComment.SpacingAfterComment)
 7 | ERROR | Missing @package tag in file comment
   |       | (Squiz.Commenting.FileComment.MissingPackageTag)
----------------------------------------------------------------------

[jrfnl]

Those all look legitimate and easy enough to fix.

If the extensions directive in the custom ruleset will be changed, I'd suggest adding an exclude pattern for minified js and css files. Even though _s does not contain these, most derivative themes probably will, so setting an exclusion for the minified css/js files will save them some grief. I'm thinking along the lines of:

<exclude-pattern>*.min.(css|js)</exclude-pattern>

[grappler]

@truongwp I have had another look at this PR.

Could you add the CSS and SCSS files to the PHPCS check? Like this:

--- phpcs.xml.dist
+++ phpcs.xml.dist
@@ -17,7 +17,7 @@
    <arg value="psvn"/>

    <!-- Only check the PHP files. JS files are checked separately with JSCS and JSHint. -->
-   <arg name="extensions" value="php"/>
+   <arg name="extensions" value="php,css,scss/css"/>

    <!-- Check all files in this directory and the directories below it. -->
    <file>.</file>

When you run PHPCS you will notice that _s/sass/modules/_clearings.scss is still missing a newline at the end of the file.

@jrfnl I have created a separate pull request for the JavaScript file issues #1187. We can add the exclude-pattern once we have merged these two pull requests.

[truongwp]

@grappler I updated the old commit to add newline at end of _clearing.scss, and added another commit to update the PHP ruleset.

[grappler]

Looks good to me. It makes sense to have two commits here.