jscs-dev / node-jscs

:arrow_heading_up: JavaScript Code Style checker (unmaintained)
https://jscs-dev.github.io
MIT License
4.96k stars 513 forks source link

requireShorthandArrowFunctions matching multi-line returns #2169

Closed mxkxf closed 8 years ago

mxkxf commented 8 years ago

Hi,

I don't think this is really a bug but would be good to clear something up.

I was working on a gulpfile this morning and thought I'd convert to use the ES2015 syntax to be in line with the rest of the codebase.

The jscs-linter for Atom kept squawking at one particular point (on the first line):

gulp.task('js', () => {
  return browserify(config.src.root + '/' + config.src.js + '/script.js')
    .transform(babelify, { presets: ['es2015', 'react'] })
    .bundle()
    .pipe(source('script.js'))
    .pipe(gulp.dest(config.dist.root + '/' + config.dist.js));
});
error requireShorthandArrowFunctions Use the shorthand arrow function form

I understand why this is squawking; the gulp task is just returning one thing, so is trying to treat it as a one-liner.

Was just wondering what your thoughts might be on this?

hzoo commented 8 years ago

Hmm I guess this is just how the rule was written so not a bug but not people expect. If you look at the source, it's pretty simple. We're just checking for a BlockStatement (use of {}) and then whether the only statement in the body is a return statement.

Not exactly sure what we should do here (we should be more lax about it somehow).

Yeah you could make a variable, or you could probably do

gulp.task('js', () => 
  browserify(config.src.root + '/' + config.src.js + '/script.js')
    .transform(babelify, { presets: ['es2015', 'react'] })
    .bundle()
    .pipe(source('script.js'))
    .pipe(gulp.dest(config.dist.root + '/' + config.dist.js))
);
mxkxf commented 8 years ago

Thanks - yes, that would work. However, there's probably some other cases where people are using chain-able, "fluent" APIs and will need to return a value, so I imagine they will have the same problem.

For the moment I've just made a variable (which wasn't an issue at all), and will keep my eye on the changelogs and update if this is ever reimagined in the future.

hzoo commented 8 years ago

Oh, part of the thing with the shorthand syntax is that it does automatically return the thing that's passed in.

arr.map((a) => a);

arr.map(function (a) {
  return a;
});

so your code should still return browserify... etc

markelog commented 8 years ago

At this point only major and CST related bugs will be fixed.