Add support for CSS validation

[shadyvb]

Something like https://www.npmjs.org/package/grunt-w3c-validation would be a great thing to have.

[westonruter]

Yes! Though perhaps a SCSS validator would be even more useful?

[westonruter]

@mehigh are you aware of a CSS/SCSS validation tool that we should incorporate into our code checks?

[westonruter]

PHP_CodeSniffer can run checks on CSS, though we haven't used them before:

https://github.com/squizlabs/PHP_CodeSniffer/tree/master/CodeSniffer/Standards/Squiz/Sniffs/CSS
https://github.com/squizlabs/PHP_CodeSniffer/tree/master/CodeSniffer/Standards/MySource/Sniffs/CSS

[westonruter]

See also: https://xwp.github.io/engineering-best-practices/css/#syntax-formatting

[ntwb]

@westonruter I'm working on adding PostCSS and Stylelint to WordPress core, hoping to get the last of this sorted next week https://core.trac.wordpress.org/ticket/29792#comment:34

Should also be able to extend to SCSS support now that PostCSS 5.0 is out

[mehigh]

@westonruter I did not use a SCSS validation tool as the compiler is the first line of defence in catching errors. The compilers never generates invalid code.

At the slightest hick-up you get very verbose descriptions of the error. This is what I got when I typed
p { h }

Message:
scss/common/_header.scss
284:6 property "h" must be followed by a ':'
Details:
column: 6
line: 284
file: /Sites/vagrant-local/www/xwp.co/wp-content/themes/xwp-reborn/scss/common/_header.scss
status: 1
messageFormatted: scss/common/_header.scss
284:6 property "h" must be followed by a ':'

That's what I have in the regular gulp processes I built on various projects. (I prefer gulp over grunt due to the more elegant syntax and more clear and concise configuration files).

[mehigh]

I gave http://csslint.net/ a try. And through 270 warnings I got from the stylesheet generated, I noticed 12 valid items which I had included in improving the code base.

But it has too big of a signal-to-noise ratio to be a recommendation.

[mehigh]

An SCSS validator would only make sense if we're compiling the SCSS files on the server, as long as this thing is happening locally, it's not a requirement to be added in a pre-commit hook.

We could potentially do a 'catch all CSS is valid sort of tool' to be plugged-in regardless on the aspects of how the CSS is built on a per-project basis, but we would only be interested here in errors, and not warnings.

As for PostCSS and it becomming a replacement for SASS, what tool I've used with success on many occasions is the AutoPrefixer to which I fed the CSS generated by the pre-processor.

Even if PostCSS 5.0 adds SCSS support, I would still maintain the recommendation to use libSass as the go-to solution for CSS compilation (there are threads on the internet talking about it being 3 times as fast than libSass, etc.). I prefer not to go on the 'bleeding edge' and hit compiler-introduced CSS bugs and stick to more stable, tested solutions. Maybe once development starts toning down and PostCSS becomes a more mature solution we can consider switching boats, but now I don't consider it to be the time.

[mehigh]

Stylelint seems to be a wee bit more modern than the CSSLint I just tried earlier.
http://benfrain.com/floss-your-style-sheets-with-stylelint/
Funny how the author went "I go friggin’ ape if someone doesn’t put a space here: display:block"
I bet he never liked the Stylus preprocessor. That's a thing I totally loved, but unfortunately didn't catch up much traction around.
So if I were to pick one I'd say Stylelint is a solid choice, and mostly because it allows a very granular configuration. And for things which don't matter at all (on whether one added a space or not after : in a SCSS source file, for example), I would just disable them.

And for things where there shouldn't be a unit defined after a 0, that's the sort of thing which shouldn't require a manual developer interaction to fix upon a lint notices it, instead there are plenty of css optimisers which are better at handling this sort of items when minifying the CSS anyway.

So as long as the rule set is adapted to catching actual issues, I think that would be good.

[westonruter]

@ntwb Does stylelint support generating reports in the same formats as ESLint (namely compact)? If so, then we could incorporate it into the same patch-checking logic like:

wp-dev-lib/check-diff.sh

Lines 758 to 776 in e52081e

	# Run ESLint.
	if [ -n "$ESLINT_CONFIG" ] && [ -e "$ESLINT_CONFIG" ] && [ -e "$(npm bin)/eslint" ] && check_should_execute 'eslint'; then
	(
	echo "## ESLint"
	cd "$LINTING_DIRECTORY"
	if ! cat "$TEMP_DIRECTORY/paths-scope-js" | remove_diff_range | xargs "$(npm bin)/eslint" --max-warnings=-1 --quiet --format=compact --config="$ESLINT_CONFIG" --output-file "$TEMP_DIRECTORY/eslint-report"; then
	if [ "$CHECK_SCOPE" == 'patches' ]; then
	cat "$TEMP_DIRECTORY/eslint-report" | php "$DEV_LIB_PATH/diff-tools/filter-report-for-patch-ranges.php" "$TEMP_DIRECTORY/paths-scope-js" | cut -c$( expr ${#LINTING_DIRECTORY} + 2 )-
	phpcs_status="${PIPESTATUS[1]}"
	if [[ $phpcs_status != 0 ]]; then
	return $phpcs_status
	fi
	elif [ -s "$TEMP_DIRECTORY/eslint-report" ]; then
	cat "$TEMP_DIRECTORY/eslint-report" | cut -c$( expr ${#LINTING_DIRECTORY} + 2 )-
	exit 1
	fi
	fi
	)
	fi

[ai]

@westonruter Stylelint is just PostCSS plugins. They uses common PostCSS warnings API — result.message. So you can just replace reporter with simple own code for any format.

@davidtheclark @hudochenkov do we have ESLint compact reporter? Also I think it is nice idea to have same output format (maybe as addition formatter).

[ntwb]

stylelint includes 3 formatters out of the box, json, string, verbose

A quick search of NPM doesn't reveal a compact formatter, nor a quick GitHub search

As @ai notes it should be pretty quick and easy to create a "compact" formatter for us to use:

https://stylelint.io/developer-guide/formatters/

I'm happy to take a shot at writing stylelint-formatter-compact following ESLints example for the community

[ntwb]

I've got this right now based on ESLints compact format:

stylelint

/Users/netweb/dev/stylelint/stylelint-config-wordpress/__tests__/values-invalid.css: line 6, col 11, error - Unexpected unit (length-zero-no-unit)

ESLint:

/Users/netweb/dev/eslint/wordpress-svn/tests/qunit/vendor/sinon.js: line 1214, col 31, Error - Strings must use singlequote. (quotes)

Can publish as stylelint-formatter-compact or add it as a custom formatter here for wp-dev-lib

[ntwb]

Initial release published https://www.npmjs.com/package/stylelint-formatter-compact

[westonruter]

Started PR #239 for adding stylelint integration. I know that it is not correctly handling the --formatter argument. Please comment on the PR.