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

Support fix option #109

Closed
wants to merge 4 commits into from
Closed

Support fix option #109

wants to merge 4 commits into from

Conversation

kui
Copy link

@kui kui commented Oct 22, 2015

Add gulpEslint.outputFixes which write fixed contents to the files.

For example:

gulp.src('js/**/*.js')
    .pipe(eslint({ fix: true })
    .pipe(eslint.outputFixes())
    .pipe(eslint.format())

kui added 2 commits October 22, 2015 13:28
Add `gulpEslint.outputFixes` to write fixed contents by CLIEngine to the files
using exaples from `CLIEngine.outputFixes`.
@shinnn
Copy link
Collaborator

shinnn commented Oct 22, 2015

No tests.

@kui
Copy link
Author

kui commented Oct 22, 2015

@shinnn
Sorry about that. I added tests.

@adametry
Copy link
Owner

I appreciate the PR and tests; however, I'm not certain of the approach. Since gulp is basically streaming transformation tool, I believe it would be better to apply the fix to file.contents and use standard gulp.dest to write to disk.

I've recently published a release candidate that includes support for the "fix" option (among other things). This pre-release version can be installed using the "rc" tag, like so:

npm install gulp-eslint@rc

If you could, try it out and see if the following approach works for you:

var gulp = require('gulp');
var eslint = require('gulp-eslint');
gulp.task('lint-fix', function () {
    return gulp.src('js/**/*.js')
        .pipe(eslint({ fix: true }))
        .pipe(eslint.format())
        .pipe(gulp.dest('./'));
});

Alternatively, here's a version that writes only writes files that eslint fixed, using gulp-if:

var gulp = require('gulp');
var eslint = require('gulp-eslint');
var gulpIf = require('gulp-if');
function isFixed(file) {
    return file.eslint && typeof file.eslint.output === 'string';
}
gulp.task('lint-fix', function () {
    return gulp.src('js/**/*.js')
        .pipe(eslint({ fix: true }))
        .pipe(eslint.format())
        .pipe(gulpIf(isFixed, gulp.dest('./')));
});

By folding the fix to the gulp stream, it leaves the possibility for additional linting, transforms, concatenation, etc. Besides, with eslint able to "fix" source now, it's becoming a transformation tool (like a rule-based browserify-lite).

@kui
Copy link
Author

kui commented Oct 23, 2015

@adametry
I think it's the better way than my patch. I'll close this.
But I want your answer before @shinnn's comment if possible. I misunderstood that this patch will be merged. I found that the tests were required by coveralls.
Thanks.

@kui kui closed this Oct 23, 2015
@grssam
Copy link

grssam commented Oct 26, 2015

Hi, I am using the rc version like this:

function isFixed(file) {
  return file.eslint && typeof file.eslint.output === 'string';
}

gulp.task("lint", function() {
  return gulp.src("js/**/*.js")
    .pipe(eslint({
      fix: true,
      rules: {
        "no-multi-spaces": 2,
        "indent": [2, 2, {"SwitchCase": 1}]
      }
    }))
    .pipe(eslint.formatEach())
    .pipe(eslint.failAfterError())
    .pipe(gulpIf(isFixed, gulp.dest("./")));
});

When I run gulp lint, it does nothing - Neither does it fix the indentation or multiple spaces, nor does it output the errors on console. If I make fix as false, it reports the errors correctly. failAfterError has no effect.

@grssam
Copy link

grssam commented Oct 26, 2015

And I found the issue - in gulp.dest you need to give the base directory. So in my case (and in the case of the example provided), it should be gulp.dest("js")

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.

4 participants