CLI: Add --esprima option. #708 #711

koistya · 2014-10-22T14:32:38Z

Currently you cannot use csjs with JSX (ReactJS) files:

var React = require('react');
module.exports = React.createClass({
  render() {
    return <div>Hello</div>;
  }
});

Unexpected token < at ./src/components/test.jsx :
      3 |  render() {
      4 |    return <div>Hello</div>;
--------------------^

If you could provide a custom Esprima version via CLI or .jscsrc file, that would fix the problem:

at the CLI:

jscs --esprima=esprima-fb

in .jscsrc

{
  "esprima": "esprima-fb"
}

coveralls · 2014-10-22T14:37:15Z

Coverage decreased (-0.04%) when pulling da794b5 on koistya:esprima into d1a246b on jscs-dev:master.

mdevils · 2014-10-22T14:41:07Z

Hello! Thank you for your submission.

StringChecker cannot include non-precalculated requires as it is used to build browser-version.

You only can do this in Checker.

Also, the base path for the require should be process.cwd().

mikesherov · 2014-10-22T15:30:58Z

Also, there needs to be tests for this, and as well, you need to actually add the esprima option to the bin/jscs

mikesherov · 2014-10-22T15:33:48Z

Thanks again @koistya. I appreciate the effort you've put forth here.

Instead of looking for a string type in constructor, you should follow the conventions we have for new options: it needs to work from both .jscsrc and the cli, it needs a module to consume the option and remove it off of the config option, and then you need a set of unit tests to make sure your change works.

coveralls · 2014-10-23T06:28:05Z

Coverage decreased (-0.03%) when pulling 4e3161b on koistya:esprima into 7965275 on jscs-dev:master.

coveralls · 2014-10-23T06:34:32Z

Coverage decreased (-0.03%) when pulling 46d0cc3 on koistya:esprima into 7965275 on jscs-dev:master.

coveralls · 2014-10-23T06:44:38Z

Coverage decreased (-0.07%) when pulling 9b8cf69 on koistya:esprima into 7965275 on jscs-dev:master.

coveralls · 2014-10-23T08:27:53Z

Coverage decreased (-0.07%) when pulling 4ae0a5a on koistya:esprima into 7965275 on jscs-dev:master.

koistya · 2014-10-23T11:55:36Z

@mikesherov, @mdevils take a look now, is this something you would be willing to merge? If so, I will try to add tests.

mikesherov · 2014-10-23T12:05:07Z

lib/checker.js

@@ -32,6 +38,11 @@ Checker.prototype.configure = function(config) {
    excludeFiles(config, this, cwd);
    additionalRules(config, this, cwd);
    esnext(config);
+    esprima(config);
+
+    if (typeof config.esprima === 'string') {


This should be done inside the options configuration module.

mikesherov · 2014-10-23T12:13:48Z

It's definitely something we'll be merging. However, there are still a few nits to work out.

Either way, you should write tests anyway because we know what the external API already is, and it will help us review what work you've done.

coveralls · 2014-10-23T12:53:47Z

Coverage decreased (-0.04%) when pulling 1b160af on koistya:esprima into 7965275 on jscs-dev:master.

mikesherov · 2014-10-23T14:17:18Z

lib/cli.js

@@ -25,7 +25,8 @@ module.exports = function(program) {
    var promise = defer.promise();
    var checker = new Checker({
        verbose: program.verbose,
-        esnext: program.esnext
+        esnext: program.esnext,
+        esprima: program.esprima


This is why I need to see some tests :-) Currently, passing a string here will explode, no?

mikesherov · 2014-10-23T14:19:21Z

@koistya, thanks again for contributing! Please write the following tests before I review again:

--esprima option to the cli.
esprima as an option in .jscsrc.

koistya · 2014-10-24T11:42:12Z

I just have tested manually with both CLI and .jscsrc option, works fine.

coveralls · 2014-10-24T11:55:21Z

Coverage decreased (-0.1%) when pulling b5309cc on koistya:esprima into 041d1ee on jscs-dev:master.

koistya · 2014-10-24T12:45:31Z

I'm wondering, why does this unit test fail with Error: timeout of 2000ms exceeded

it('should use a custom esprima provided in the config file', function(done) {
    var data = 'import { foo } from "bar";\n';

    var result = cli({
        args: [],
        'no-colors': true,
        config: 'test/data/cli/esprima.json'
    });

    process.stdin.emit('data', data);
    process.stdin.emit('end');

    return result.promise.always(function() {
        assert(console.log.getCall(0).args[0] === 'No code style errors found.');
        rAfter();
        done();
    });
});

mikesherov · 2014-10-24T13:40:14Z

test/cli.js

+
+        beforeEach(function() {
+            sinon.spy(console, 'log');
+            process.stdin.isTTY = false;


why is this necessary?

Just have copied from the existing test above.

remove it then. :-)

mikesherov · 2014-10-24T13:41:46Z

I'm wondering, why does this unit test fail with Error: timeout of 2000ms exceeded

Maybe has something to do with setting isTTY unnecessarily?

mikesherov · 2014-10-24T13:56:20Z

lib/cli.js

@@ -113,6 +113,12 @@ module.exports = function(program) {
        return returnArgs;
    }

+    if (program.esprima && typeof program.esprima === 'string') {


this can just be if (typeof program.esprima === 'string') {

koistya · 2014-10-24T13:56:22Z

Removing isTTY didn't make any difference.

mikesherov · 2014-10-24T13:56:45Z

lib/cli.js

@@ -113,6 +113,12 @@ module.exports = function(program) {
        return returnArgs;
    }

+    if (program.esprima && typeof program.esprima === 'string') {
+        config.esprima = require(program.esprima);
+    } else if (config.esprima && typeof config.esprima === 'string') {


this can just be } else if (typeof config.esprima === 'string') {

Right, I guess config is always defined at this point.

mikesherov · 2014-10-24T13:59:18Z

@koistya thanks for sticking with this.

mikesherov · 2014-10-24T14:06:14Z

@koistya perhaps it's because you haven't added any rules?

koistya · 2014-10-26T17:38:17Z

@mikesherov I just copied a unit test from above and replaced the input string with import { foo } from "bar"; It fails with a timeout error. I checked the code manually, it works with both --esprima CLI flag and "esprima" setting in .jscsrc config file (valided JSX code by using 'esprima-fb' esprima setting). But I don't know how to make the unit test work.

mikesherov · 2014-10-26T17:59:44Z

@koistya, no worries. I'll take it the rest of way. Thanks again for contributing!

markelog · 2014-11-05T01:02:38Z

@mikesherov ?

mikesherov · 2014-11-05T03:26:24Z

Was waiting for config to land. Will pick this up tomorrow.

mikesherov · 2014-11-24T13:28:53Z

@koistya Thanks again for contributing! I landed a modified patch here: bdc5ff0 Still giving you the credit though as you did a majority of the work. Thanks again!

mrjoelkemp force-pushed the master branch from 39bcf2d to 7965275 Compare October 22, 2014 23:06

koistya force-pushed the esprima branch from da794b5 to 4e3161b Compare October 23, 2014 06:25

koistya force-pushed the esprima branch from 4e3161b to 46d0cc3 Compare October 23, 2014 06:31

koistya force-pushed the esprima branch from 46d0cc3 to 9b8cf69 Compare October 23, 2014 06:41

koistya changed the title ~~Allow to specify a esprima version via CLI. #708~~ CLI: Add --esprima option. #708 Oct 23, 2014

koistya force-pushed the esprima branch 3 times, most recently from 890e096 to 4ae0a5a Compare October 23, 2014 08:23

mikesherov reviewed Oct 23, 2014
View reviewed changes

koistya force-pushed the esprima branch from 4ae0a5a to 1b160af Compare October 23, 2014 12:50

mikesherov reviewed Oct 23, 2014
View reviewed changes

markelog force-pushed the master branch from f4048a3 to 041d1ee Compare October 23, 2014 17:42

markelog added cli new option labels Oct 23, 2014

koistya force-pushed the esprima branch 3 times, most recently from 4d223f4 to b5309cc Compare October 24, 2014 11:49

koistya force-pushed the esprima branch 3 times, most recently from 85d811a to 4833995 Compare October 24, 2014 12:44

koistya force-pushed the esprima branch from 4833995 to d71828e Compare October 24, 2014 12:46

mikesherov reviewed Oct 24, 2014
View reviewed changes

koistya force-pushed the esprima branch from d71828e to 3ff7dd9 Compare October 24, 2014 13:55

mikesherov reviewed Oct 24, 2014
View reviewed changes

Allow to specify a esprima version via CLI. jscs-dev#708

90e5666

koistya force-pushed the esprima branch from 3ff7dd9 to 90e5666 Compare October 24, 2014 14:16

mrjoelkemp force-pushed the master branch from 7773546 to 086031e Compare October 29, 2014 01:24

mikesherov closed this in bdc5ff0 Nov 24, 2014

yannickcr mentioned this pull request Nov 24, 2014

Regexp tokens are out of order (this shows the error not the fix) facebookarchive/esprima#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Add --esprima option. #708 #711

CLI: Add --esprima option. #708 #711

koistya commented Oct 22, 2014

coveralls commented Oct 22, 2014

mdevils commented Oct 22, 2014

mikesherov commented Oct 22, 2014

mikesherov commented Oct 22, 2014

coveralls commented Oct 23, 2014

coveralls commented Oct 23, 2014

coveralls commented Oct 23, 2014

coveralls commented Oct 23, 2014

koistya commented Oct 23, 2014

mikesherov Oct 23, 2014

mikesherov commented Oct 23, 2014

coveralls commented Oct 23, 2014

mikesherov Oct 23, 2014

mikesherov commented Oct 23, 2014

koistya commented Oct 24, 2014

coveralls commented Oct 24, 2014

koistya commented Oct 24, 2014

mikesherov Oct 24, 2014

koistya Oct 24, 2014

mikesherov Oct 24, 2014

mikesherov commented Oct 24, 2014

mikesherov Oct 24, 2014

koistya commented Oct 24, 2014

mikesherov Oct 24, 2014

koistya Oct 24, 2014

mikesherov commented Oct 24, 2014

mikesherov commented Oct 24, 2014

koistya commented Oct 26, 2014

mikesherov commented Oct 26, 2014

markelog commented Nov 5, 2014

mikesherov commented Nov 5, 2014

mikesherov commented Nov 24, 2014

CLI: Add --esprima option. #708 #711

CLI: Add --esprima option. #708 #711

Conversation

koistya commented Oct 22, 2014

coveralls commented Oct 22, 2014

mdevils commented Oct 22, 2014

mikesherov commented Oct 22, 2014

mikesherov commented Oct 22, 2014

coveralls commented Oct 23, 2014

coveralls commented Oct 23, 2014

coveralls commented Oct 23, 2014

coveralls commented Oct 23, 2014

koistya commented Oct 23, 2014

Choose a reason for hiding this comment

mikesherov commented Oct 23, 2014

coveralls commented Oct 23, 2014

Choose a reason for hiding this comment

mikesherov commented Oct 23, 2014

koistya commented Oct 24, 2014

coveralls commented Oct 24, 2014

koistya commented Oct 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesherov commented Oct 24, 2014

Choose a reason for hiding this comment

koistya commented Oct 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesherov commented Oct 24, 2014

mikesherov commented Oct 24, 2014

koistya commented Oct 26, 2014

mikesherov commented Oct 26, 2014

markelog commented Nov 5, 2014

mikesherov commented Nov 5, 2014

mikesherov commented Nov 24, 2014