Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Changed .csslintrc file support to use CSSLint specification #6

Closed
wants to merge 7 commits into from

7 participants

@stevenbenner

This pull request is related to the discussion in issue #3.

What this patch adds

  • Support for .csslintrc files in their native format.
  • The ability to disable linting rules via the --ignore directive.
  • The ability to exclude specific files from linting via the --exclude-list directive.
  • The ability to set the warning level of rules via the --errors and --warnings directives. (But they don't really affect this grunt task because even warnings will cause the task to fail and all rules are set to warnings by default)

What this patch does not add

The official .csslintrc file spec allows for any command line option. I left out support for the ones that I didn't think there was any reason to support. These include:

  • --help - Obviously pointless in a grunt task.
  • --format - Implementing this really didn't make sense to me because it doesn't really lend itself to working with the way formatters work now.
  • --list-rules - Doesn't belong in a grunt task.
  • --quiet - Would anyone really like for this to affect a grunt task?
  • --version - Another one that obviously doesn't belong in a grunt task.
  • It is also possible to add files to be linted via the .csslintrc file. I was debating whether I should implement that or not, but I decided against it because grunt users must explicitly list the files/directories that they want the task to run on.

There is certainly room for discussion about supporting the omitted features. Let me know your thoughts.

Behavior changed by this patch

  • This pull request drops support for JSON .csslintrc files completely. Anyone who has built a JSON .csslintrc file will find their csslint task broken when they upgrade.

Code notes

  • I added the .csslintrc parsing code in a file outside of the core task js. It felt more like a small library when I was writing it.
  • It would be much better to use CSSLint to process config files, but CSSLint does not expose any of the CLI code where the .csshintrc related code resides. Perhaps we should consider requesting (or submitting a pull request) that CSSLint exposes the argument processing code somewhere.
  • The rcparser exposes several convenience methods. These methods centralize the maintenance points that need to be touched if CSSLint ever changes their argument names or spec. However these might just be wasted code depending on how you look at it.
  • I used the node path library to determine the basePath for the .csslintrc file and to do the file exclusion comparisons because that just feels right, but I can't find anywhere else in grunt that behaves this way. This might be an undesirable practice in grunt plugins for some reason.
@jzaefferer
Collaborator

I veto against landing this until we at least petition the CSSLint folks to change the format of their .csslintrc file to be consistent with what jshint does, and what we implemented now. That format is much more sane and works nicely when moving from options to rc file or back: jquery/jquery-ui@39a2f46

@tkellen
Owner

I'll hold off on this until you've done so. Would you please /cc me on the issue you make over @ CSSLint?

@asciidisco

@jzaefferer Have you pinged @stubbornella or @nzakas already?
I think the jshintrc style is definitely an improvement in readability, it might be overhead, but we could do a type checking of the csslintrc contents and pick the right parser.

Another thing is, if any of the editor plugin developers wrote its own implementation of a csslintrc parser instead of using the CSSLint internal stuff, they have to change it also. I'm totally behind the idea, but I think it will be a rocky road.

@stevenbenner
Nicely done PR (in fact much better than any PR I've ever done), will scan the code later this evening.

@stevenbenner

@jzaefferer @tkellen Any news on this?

If you guys are waiting for a resolution on getting CSSLint to support s JSON configuration file then I think that at the very least the existing JSON support implemented in this plugin should be removed because it is very unlikely that whatever spec CSSLint settles on will be the same as we find in the current version of this plugin.

When you search GitHub for .csslintrc you will find that there is a growing number of repositories that have implemented .csslintrc files with a spec that exists nowhere in the universe except for grunt-contrib-csslint.

Like it or not, it is important to understand that official grunt plugins (and jQuery UI which is using this plugin) have some serious clout in the JavaScript world and people assume that the way it's done in these repositories is the right way to do things.

@jzaefferer
Collaborator

@stevenbenner no news, no, except that Nicholas declared not being able to deal with CSSLint at all. It looks like the last commit from Nicole was in December and there were no commits for two months now. So expecting anything to change within CSSLint is pretty unrealistic at this point.

The SublimerLinter devs would also favor a JSON format that doesn't require a custom parser.

As for this comment:

whatever spec CSSLint settles on will be the same as we find in the current version of this plugin

You may be right, but I'd like to at least point out that this format is actually used by CSSLint itself. They don't support it for the .csslintrc file, but its exactly what the programmatic API uses. Which explains why there's very little code in the plugin to convert the format in use to what CSSLint expects.

Anyway, since CSSLint isn't moving anywhere, merging this PR is probably the sanest thing to do.

@stubbornella

So, help me catch up. It seems that several people would like to change the way configuration is done in CSS Lint to match how it is done in JSHint, am I right? But doing so would mean we needed to parse json?

So, I can see the value in the former, how big a pain point is the latter?

@jzaefferer
Collaborator

Parsing and outputting JSON is trivial, since we can just use JSON.parse and JSON.stringify. The more interesting question then is how to actually structure the data. The format CSSLint uses internally has a key per rule and a value to set each rule to ignore, warn or error, like this:

{
  "qualified-headings": true,
  "unique-headings": true,
  "known-properties": false
}

The CLI arguments have lists of rules for each of these states, which this PR uses:

--ignore=box-model,ids,important
--warn=unique-headings

Switching to JSON could mean using that first format, or converting that second format to a JSON structure, for example like this:

{
  "ignore": ["box-model", "ids", "important"],
  "warn": ["unique-headings"]
}

The first format, which this plugin currently (incorrectly) uses matches the format used by JSHint as well as what CSSLint uses internally.

The last format is also what Scott suggested in his comment here. He even has a (trivial) patch to add JSON support in addition of the current format.

@robwierzbowski

The rule key/value pair is a common pattern, and more human readable and versionable. +1 from me.

@sindresorhus
Collaborator

ping

merge conflict needs to be resolved.

@stevenbenner

@sindresorhus Are you pinging me?

I see that support for JSON config files just got added to CSSLint, and will be available in the next release. That probably makes this pull request redundant. But if you would like to add support for the old fashion config files I suppose I could revisit this.

@sindresorhus
Collaborator

@stevenbenner yes, just wanted to clean up some dormant prs. we'll wait for the new csslint release.

@stevenbenner stevenbenner deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 Gruntfile.js
@@ -33,6 +33,12 @@ module.exports = function(grunt) {
'ids': 0
},
files: ['test/fixtures/invalid.css']
+ },
+ external: {
+ options: {
+ csslintrc: 'test/fixtures/.csslintrc'
+ },
+ src: ['test/fixtures/*.css']
}
}
});
@@ -45,7 +51,7 @@ module.exports = function(grunt) {
grunt.loadNpmTasks('grunt-contrib-internal');
// plugin's task(s), manually check the output, then run `grunt csslint:all` and `grunt csslint:custom` to look at lint errors
- grunt.registerTask('test', ['csslint:valid', 'csslint:empty']);
+ grunt.registerTask('test', ['csslint:valid', 'csslint:empty', 'csslint:external']);
// By default, lint and run all tests.
grunt.registerTask('default', ['jshint', 'test', 'build-contrib']);
View
14 docs/csslint-options.md
@@ -53,15 +53,15 @@ A few additional options are supported:
Type: `String`
Default value: `null`
-If this filename is specified, options and globals defined therein will be used. Task and target options override the options within the `csslintrc` file. The `csslint` file must be valid JSON and looks something like this:
+If this filename is specified the options defined therein will be used. Task and target options override the options within the `csslintrc` file. The `csslintrc` file must be a series of [CSSLint command line arguments](https://github.com/stubbornella/csslint/wiki/Command-line-interface) separated by whitespace or linefeeds.
-```json
-{
- "qualified-headings": true,
- "unique-headings": true,
- "known-properties": false
-}
```
+--ignore=box-model,ids,important
+--exclude-list=invalid.css,empty.css
+```
+
+This task only processes the `--ignore`, ` --exclude`, `--errors`, and `--warnings` declarations.
+
#### formatters
Type: `array`
Default value: `null`
View
23 tasks/csslint.js
@@ -11,15 +11,29 @@
module.exports = function(grunt) {
grunt.registerMultiTask( "csslint", "Lint CSS files with csslint", function() {
var csslint = require( "csslint" ).CSSLint;
+ var rcparser = require( "./lib/rcparser" ).init( grunt );
var ruleset = {};
var verbose = grunt.verbose;
var externalOptions = {};
var combinedResult = {};
var options = this.options();
+ var filesList = this.filesSrc;
- // Read JSHint options from a specified jshintrc file.
+ // Read CSSLint options from a specified csslintrc file.
if (options.csslintrc) {
- externalOptions = grunt.file.readJSON( options.csslintrc );
+ rcparser.parse( options.csslintrc );
+
+ externalOptions = rcparser.getOptionsObject();
+
+ // filter out excluded files
+ filesList = filesList.filter(function( file ) {
+ if ( rcparser.isFileExcluded( file ) ) {
+ verbose.writeln( "Skipping file excluded by CSSLint config " + file );
+ return false;
+ }
+ return true;
+ });
+
// delete csslintrc option to not confuse csslint if a future release
// implements a rule or options on its own
delete options.csslintrc;
@@ -39,8 +53,9 @@ module.exports = function(grunt) {
ruleset[ rule ] = options[ rule ];
}
}
+
var hadErrors = 0;
- this.filesSrc.forEach(function( filepath ) {
+ filesList.forEach(function( filepath ) {
var file = grunt.file.read( filepath ),
message = "Linting " + filepath + "...",
result;
@@ -92,6 +107,6 @@ module.exports = function(grunt) {
if ( hadErrors ) {
return false;
}
- grunt.log.ok( this.filesSrc.length + " files lint free." );
+ grunt.log.ok( filesList.length + " files lint free." );
});
};
View
98 tasks/lib/rcparser.js
@@ -0,0 +1,98 @@
+'use strict';
+
+var path = require('path');
+
+exports.init = function(grunt) {
+
+ var exports = {},
+ statements = {},
+ basePath = '',
+ excludeList = [];
+
+ // parses a .csslintrc file into an object
+ exports.parse = function(filePath) {
+ var data = grunt.file.read(filePath);
+ basePath = path.dirname(filePath);
+
+ grunt.verbose.write('Parsing CSSLint config ' + filePath + '...');
+
+ // walk through the individual statements
+ data.split(/[\s\n\r]+/m).forEach(function(statement) {
+
+ // ignore empty lines
+ if (!statement) {
+ return;
+ }
+
+ // if a statement begins with '--' then it is an option
+ if (statement.indexOf('--') === 0) {
+ // the options are key/value pairs
+ // and the value is optional with a default of true
+ var pair = statement.substring(2).split('=');
+ if (pair.length === 1) {
+ statements[pair[0]] = true;
+ } else {
+ statements[pair[0]] = pair[1].split(',');
+ }
+ }
+
+ });
+
+ // build the path-based file exclude list for easy lookups
+ excludeList = exports.getExcludeList().map(function(file) {
+ return path.join(basePath, file);
+ });
+
+ grunt.verbose.ok();
+ return statements;
+ };
+
+ // returns a options object, ready to be merged into the task options
+ exports.getOptionsObject = function() {
+ var options = {};
+
+ // 2 = error, 1 = warning, 0 = ignore
+ exports.getErrorsList().forEach(function(rule) {
+ options[rule] = 2;
+ });
+ exports.getWarningsList().forEach(function(rule) {
+ options[rule] = 1;
+ });
+ exports.getIgnoreList().forEach(function(rule) {
+ options[rule] = 0;
+ });
+
+ return options;
+ };
+
+ // checks if a file is in the exclude list
+ exports.isFileExcluded = function(filePath) {
+ if (grunt.util._.indexOf(excludeList, path.normalize(filePath)) !== -1) {
+ return true;
+ }
+ return false;
+ };
+
+ // gets the list of error level rules
+ exports.getErrorsList = function() {
+ return statements['errors'] || [];
+ };
+
+ // gets the list of warning level rules
+ exports.getWarningsList = function() {
+ return statements['warnings'] || [];
+ };
+
+ // gets the list of ignored rules
+ exports.getIgnoreList = function() {
+ return statements['ignore'] || [];
+ };
+
+ // gets the list of excluded files
+ exports.getExcludeList = function() {
+ return statements['exclude-list'] || [];
+ };
+
+ return exports;
+
+};
View
4 test/fixtures/.csslintrc
@@ -0,0 +1,4 @@
+--ignore=box-model,ids,important
+--errors=import,box-sizing
+--warnings=star-property-hack
+--exclude-list=invalid.css,empty.css
View
21 test/fixtures/warning.css
@@ -0,0 +1,21 @@
+/* warning: "Don't use IDs in selectors" */
+#idInSelector {
+ font-size: 12pt;
+}
+
+/* warning: "Don't use width or height when using padding or border" */
+.boxModel {
+ border: 1px solid black;
+ padding: 5px;
+ width: 100px;
+}
+
+/* warning: "Be careful when using !important declaration" */
+.important {
+ color: red !important;
+}
+
+/* this will not cause any warnings */
+.perfectlyValid {
+ background-color: #eee;
+}
Something went wrong with that request. Please try again.