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

Add ability to exclude files #3

Closed
wants to merge 2 commits into from
Closed

Add ability to exclude files #3

wants to merge 2 commits into from

Conversation

darylldoyle
Copy link

This will give users the ability to exclude files from linting by passing them in the exclude option. Useful when using third party libraries that aren't covered by your coding standards.

@noahmiller
Copy link
Owner

Thanks Daryll. It makes sense to allow users to exclude files from linting. But what does using SCSS-Lint's exclude option allow that isn't possible through negated patterns, which seems like more idiomatic gulp usage? For example:

gulp.src(['./styles/*.scss', '!folder/_file.scss'])
  .pipe(scsslint())
});

Also, the exclude option can be used in an .scss-lint.yml file:

exclude: 'app/assets/stylesheets/plugins/**'

@darylldoyle
Copy link
Author

Hi Noah. In my opinion, you're completely correct. My thoughts for it were if people decide to use it in their sass compilation task, E.G.

gulp.src('./styles/*.scss')
    .pipe(sass({errLogToConsole: true}))
    .pipe(scsslint({exclude:'./styles/_normalize.scss'}))
    .pipe(gulp.dest('assets/css'))

I have seen a lot of people new to gulp arrange each of their tasks into one long list of pipes and in this case, to me, it seems more logical to allow them to exclude files, rather than forcing them to break their file down.

@noahmiller
Copy link
Owner

That makes sense and is straight-forward for new gulp users, but there are two issues I see:

  1. If the set of input files happens to be contained in the set of excluded files, then scss-lint returns error code 66. We can't ignore that error code, since it's also used to mean a file wasn't readable. Though this case seems relatively rare, it might occur if a file change watch task is active and a user happens to change one of the excluded files but no other files. gulp-scsslint would then be passed just the changed file and would return an error.
  2. We add scsslint objects to each file in the stream indicating success or failure. scss-lint only tells us which files had errors, not which passed or were excluded. Hence we'd end up adding file.scsslint = {success: true} to files that didn't actually get linted, which would be misleading.

I'm not sure how to work around either issue, or that a complex work-around would be worthwhile.

It might be better to recommend that people either use two tasks with two different srcs, or use gulp-filter like:

var gulpFilter = require('gulp-filter');
var filter = filter('!./styles/_normalize.scss');

gulp.src('./styles/*.scss')
    .pipe(sass({errLogToConsole: true}))
    .pipe(filter)
    .pipe(scsslint())
    .pipe(filter.restore())
    .pipe(gulp.dest('assets/css'))

What do you think? We can document either or both of these use cases in the ReadMe if they seem like reasonable options to avoid the problems above.

@darylldoyle
Copy link
Author

The first point you make completely slipped my mind and is quite an issue.

Whilst telling people to use gulp-filter would be the best way around it, I then think it again complicates the issue for new gulp users, which sort of defeats the object of adding the exclude in the first place.

Maybe it would be worth documenting both issues in the ReadMe as you suggest and advising people to use the recommended setup, with two tasks. But leaving the exclude there, with the warning that it can produce the above error.

The way I see it, if a new user only wants to exclude a couple of included files, such as third party scripts, this may be an easy way for them, but the more complex the setup, the more I'd recommend using multiple tasks.

@noahmiller
Copy link
Owner

I've updated the plugin with commit d8dbb47 to document best practices for excluding files, and with commit 4347490 to support passing any scss-lint supported options. With the second commit, it's possible to use the exclude option like so:

scsslint({ args: ['--exclude=vendor.scss'] })

but with documented caveats and warnings.

I think this satisfies both making it easier for new users and providing full (if not recommended and not fully supported) access to scss-lint's command line options.

@noahmiller noahmiller closed this May 16, 2014
@noahmiller
Copy link
Owner

By the way, thanks for the great discussion and insight into this project! I appreciate getting your input on it.

@darylldoyle
Copy link
Author

That seems like a good option, seems to work well! Thanks to you too, for both the discussion and for creating the plugin. I for one find it mighty useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants