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

--config fails with non .json file #486

Closed
aparajita opened this Issue Jan 2, 2014 · 26 comments

Comments

Projects
None yet
7 participants
@aparajita
Contributor

aparajita commented Jan 2, 2014

This fails:

>> eslint --config eslint.conf test.js
/Users/aparajita/Development/Libs/Sublime Text 3/atest/eslint.conf:2
    "env": {
         ^
SyntaxError: Unexpected token :
...

This works:

>> eslint --config eslint.json test.js
test.js: line 5, col 14, Error - 'bar' is not defined.
@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Jan 2, 2014

Member

It looks like this error is due to require filing to parse file correctly. I can't seem to find any information about CommonJS and non-javascript/json files. Not sure how we can fix it, other then to parse options file manually.

Member

ilyavolodin commented Jan 2, 2014

It looks like this error is due to require filing to parse file correctly. I can't seem to find any information about CommonJS and non-javascript/json files. Not sure how we can fix it, other then to parse options file manually.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 2, 2014

Contributor

Using require to parse JSON is perhaps not such a great idea. Strange you chose to do it that way, since you already do it the correct way in getLocalConfig:

config = JSON.parse(fs.readFileSync(localConfigFile));
Contributor

aparajita commented Jan 2, 2014

Using require to parse JSON is perhaps not such a great idea. Strange you chose to do it that way, since you already do it the correct way in getLocalConfig:

config = JSON.parse(fs.readFileSync(localConfigFile));
@neonstalwart

This comment has been minimized.

Show comment
Hide comment
@neonstalwart

neonstalwart Jan 2, 2014

it's not CommonJS-specific but node's require parses .json files as JSON. i think manually parsing it would be the better way to do it.

neonstalwart commented Jan 2, 2014

it's not CommonJS-specific but node's require parses .json files as JSON. i think manually parsing it would be the better way to do it.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 2, 2014

Contributor

require is more finicky about paths, it's much safer to use JSON.parse. It's true that node can parse JSON, but it's hardly optimized for that, and I'll wager it ends up calling JSON.parse internally.

Contributor

aparajita commented Jan 2, 2014

require is more finicky about paths, it's much safer to use JSON.parse. It's true that node can parse JSON, but it's hardly optimized for that, and I'll wager it ends up calling JSON.parse internally.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 2, 2014

Contributor

I'm working on a fix.

Contributor

aparajita commented Jan 2, 2014

I'm working on a fix.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 3, 2014

Member

require() for JSON files is an optimization for developer happiness in that
it saves a bunch of lines of code (and a try-catch). I don't mind changing
to read the file in the more verbose way.

Member

nzakas commented Jan 3, 2014

require() for JSON files is an optimization for developer happiness in that
it saves a bunch of lines of code (and a try-catch). I don't mind changing
to read the file in the more verbose way.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 3, 2014

Contributor

I guess it comes down to whether you are willing to limit user choice. If you are, then require is fine, but it doesn't save you any code, because you have to ensure the file has a .json extension, and you still have to catch exceptions that in throws for malformed JSON. So here are the choices:

// Simple way, works with any filename
try {
    json = JSON.parse(fs.readFileSync(filePath));
} catch (e) {
    // whatever
}

// Magical require way, only works with .json files
if (path.extname(filePath) !== ".json") {
     // Stupid user, who said you could use a non .json file!
} else {
    try {
        require(fs.readFileSync(filePath));
    } catch (e) {
        // oops, we still have to catch malformed JSON
    }
}

BTW, here's the magic require is doing internally (from the node source):

Module._extensions['.json'] = function(module, filename) {
  var content = NativeModule.require('fs').readFileSync(filename, 'utf8');
  try {
    module.exports = JSON.parse(stripBOM(content));
  } catch (err) {
    err.message = filename + ': ' + err.message;
    throw err;
  }
};

So you have to duplicate what require is doing to catch malformed JSON.

Limiting user choice and increasing lines of source code doesn't seem to me to be either user happiness nor developer happiness, but that's just me.

Contributor

aparajita commented Jan 3, 2014

I guess it comes down to whether you are willing to limit user choice. If you are, then require is fine, but it doesn't save you any code, because you have to ensure the file has a .json extension, and you still have to catch exceptions that in throws for malformed JSON. So here are the choices:

// Simple way, works with any filename
try {
    json = JSON.parse(fs.readFileSync(filePath));
} catch (e) {
    // whatever
}

// Magical require way, only works with .json files
if (path.extname(filePath) !== ".json") {
     // Stupid user, who said you could use a non .json file!
} else {
    try {
        require(fs.readFileSync(filePath));
    } catch (e) {
        // oops, we still have to catch malformed JSON
    }
}

BTW, here's the magic require is doing internally (from the node source):

Module._extensions['.json'] = function(module, filename) {
  var content = NativeModule.require('fs').readFileSync(filename, 'utf8');
  try {
    module.exports = JSON.parse(stripBOM(content));
  } catch (err) {
    err.message = filename + ': ' + err.message;
    throw err;
  }
};

So you have to duplicate what require is doing to catch malformed JSON.

Limiting user choice and increasing lines of source code doesn't seem to me to be either user happiness nor developer happiness, but that's just me.

@neonstalwart

This comment has been minimized.

Show comment
Hide comment
@neonstalwart

neonstalwart Jan 3, 2014

I started in favor of manually reading the file and parsing as JSON and I think it's still my preference but there is one thing require offers that parsing JSON does not provide. require lets you use a .js module to provide your config. that makes it possible to do something more than what JSON can express - eg your .js config file might import a "base" set of config options for your organization and tweak a few of them that are specific to this project and then provide that as it's exports. JSON does not have the ability to do that. before settling on moving away from require I just want to make sure that point is considered.

ben...

On Jan 2, 2014, at 19:32, Aparajita Fishman notifications@github.com wrote:

I guess it comes down to whether you are willing to limit user choice. If you are, then require is fine, but it doesn't save you any code, because you have to ensure the file has a .json extension, and you still have to catch exceptions that in throws for malformed JSON. So here are the choices:

// Simple way, works with any filename
try {
json = JSON.parse(fs.readFileSync(filePath));
} catch (e) {
// whatever
}

// Magical require way, only works with .json files
if (path.extname(filePath) !== ".json") {
// Stupid user, who said you could use a non .json file!
} else {
try {
require(fs.readFileSync(filePath));
} catch (e) {
// oops, we still have to catch malformed JSON
}
}
BTW, here's the magic require is doing internally (from the node source):

Module._extensions['.json'] = function(module, filename) {
var content = NativeModule.require('fs').readFileSync(filename, 'utf8');
try {
module.exports = JSON.parse(stripBOM(content));
} catch (err) {
err.message = filename + ': ' + err.message;
throw err;
}
};
So you have to duplicate what require is doing to catch malformed JSON.

Limiting user choice and increasing lines of source code doesn't seem to me to be either user happiness nor developer happiness, but that's just me.


Reply to this email directly or view it on GitHub.

neonstalwart commented Jan 3, 2014

I started in favor of manually reading the file and parsing as JSON and I think it's still my preference but there is one thing require offers that parsing JSON does not provide. require lets you use a .js module to provide your config. that makes it possible to do something more than what JSON can express - eg your .js config file might import a "base" set of config options for your organization and tweak a few of them that are specific to this project and then provide that as it's exports. JSON does not have the ability to do that. before settling on moving away from require I just want to make sure that point is considered.

ben...

On Jan 2, 2014, at 19:32, Aparajita Fishman notifications@github.com wrote:

I guess it comes down to whether you are willing to limit user choice. If you are, then require is fine, but it doesn't save you any code, because you have to ensure the file has a .json extension, and you still have to catch exceptions that in throws for malformed JSON. So here are the choices:

// Simple way, works with any filename
try {
json = JSON.parse(fs.readFileSync(filePath));
} catch (e) {
// whatever
}

// Magical require way, only works with .json files
if (path.extname(filePath) !== ".json") {
// Stupid user, who said you could use a non .json file!
} else {
try {
require(fs.readFileSync(filePath));
} catch (e) {
// oops, we still have to catch malformed JSON
}
}
BTW, here's the magic require is doing internally (from the node source):

Module._extensions['.json'] = function(module, filename) {
var content = NativeModule.require('fs').readFileSync(filename, 'utf8');
try {
module.exports = JSON.parse(stripBOM(content));
} catch (err) {
err.message = filename + ': ' + err.message;
throw err;
}
};
So you have to duplicate what require is doing to catch malformed JSON.

Limiting user choice and increasing lines of source code doesn't seem to me to be either user happiness nor developer happiness, but that's just me.


Reply to this email directly or view it on GitHub.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 3, 2014

Contributor

That's wonderful, but in the meantime at least document the current limitations require imposes on the --config filename.

Contributor

aparajita commented Jan 3, 2014

That's wonderful, but in the meantime at least document the current limitations require imposes on the --config filename.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 3, 2014

Contributor

My motivation is very simple: SublimeLinter. I want to get a linter plugin working for eslint, because it's a great linter. With every other linter I have dealt with -- jshint, csslint, rubocop, you name it -- they either start looking for .rc files from the current working directory (as opposed to the linted file's directory), or they have a --config option that has no naming restrictions, which allows me to look for the .rc file myself -- simulating the linter's behavior -- and then pass the path via --config.

Right now I'm stuck because eslint searches from the file's directory, not the current working directory, and it won't let me pass .eslintrc as a config filename. Searching from the file's directory doesn't work because I'm using temp files so the plugin can lint while the user types. Of course I'd rather not use temp files, but eslint doesn't accept stdin, but that's another issue...

Contributor

aparajita commented Jan 3, 2014

My motivation is very simple: SublimeLinter. I want to get a linter plugin working for eslint, because it's a great linter. With every other linter I have dealt with -- jshint, csslint, rubocop, you name it -- they either start looking for .rc files from the current working directory (as opposed to the linted file's directory), or they have a --config option that has no naming restrictions, which allows me to look for the .rc file myself -- simulating the linter's behavior -- and then pass the path via --config.

Right now I'm stuck because eslint searches from the file's directory, not the current working directory, and it won't let me pass .eslintrc as a config filename. Searching from the file's directory doesn't work because I'm using temp files so the plugin can lint while the user types. Of course I'd rather not use temp files, but eslint doesn't accept stdin, but that's another issue...

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Jan 3, 2014

Member

@neonstalwart While it's a good point, but .eslintrc should be a configuration file, not functional. So it makes sense to be JSON, and not JavaScript. We also already have a way to inherit configs. .eslintrc will inherit (and override) eslint.json config. I think it should be fine to replace require with JSON.parse
Btw, @aparajita have you seen this? https://github.com/roadhump/SublimeLinter-eslint @neonstalwart found it earlier today. Not sure if it helps.

Member

ilyavolodin commented Jan 3, 2014

@neonstalwart While it's a good point, but .eslintrc should be a configuration file, not functional. So it makes sense to be JSON, and not JavaScript. We also already have a way to inherit configs. .eslintrc will inherit (and override) eslint.json config. I think it should be fine to replace require with JSON.parse
Btw, @aparajita have you seen this? https://github.com/roadhump/SublimeLinter-eslint @neonstalwart found it earlier today. Not sure if it helps.

@roadhump

This comment has been minimized.

Show comment
Hide comment
@roadhump

roadhump Jan 3, 2014

Contributor

@ilyavolodin this is plugin which @aparajita helps me to develop and where we face problem with config files.

Contributor

roadhump commented Jan 3, 2014

@ilyavolodin this is plugin which @aparajita helps me to develop and where we face problem with config files.

@neonstalwart

This comment has been minimized.

Show comment
Hide comment
@neonstalwart

neonstalwart Jan 3, 2014

😄 we have now come full circle - i found SL-eslint because i saw @aparajita was active here and figured he was working on an eslint plugin for SL3. anyhow, sounds like manually parsing JSON seems to be the consensus and that makes sense.

neonstalwart commented Jan 3, 2014

😄 we have now come full circle - i found SL-eslint because i saw @aparajita was active here and figured he was working on an eslint plugin for SL3. anyhow, sounds like manually parsing JSON seems to be the consensus and that makes sense.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 3, 2014

Contributor

Wonderful. I already implemented manual parsing and stdin support, but I don't have time to make it conform to the rigorous standards this project uses for submissions (which you are correct to do). It only took an hour to write the code, maybe someone else can spend an hour taking that code, adding unit tests, and re-submitting it in a form that will be accepted.

Contributor

aparajita commented Jan 3, 2014

Wonderful. I already implemented manual parsing and stdin support, but I don't have time to make it conform to the rigorous standards this project uses for submissions (which you are correct to do). It only took an hour to write the code, maybe someone else can spend an hour taking that code, adding unit tests, and re-submitting it in a form that will be accepted.

@roadhump

This comment has been minimized.

Show comment
Hide comment
@roadhump

roadhump Jan 3, 2014

Contributor

@aparajita I feel myself obliged and will re-submit code with tests soon.

Contributor

roadhump commented Jan 3, 2014

@aparajita I feel myself obliged and will re-submit code with tests soon.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@a-laughlin

This comment has been minimized.

Show comment
Hide comment
@a-laughlin

a-laughlin Jan 6, 2014

Writing this here since the error is identical. Win 7. Git bash 1.8.0. ESLint 0.2.0.

I had the problem with 0.2.0 not parsing .eslintrc. Switching the filename to eslint.json fixed that. eslint now parses the config correctly. However, the same problem is happening with all other .json files. For example, in node_modules/eslint/conf/ , running eslint eslint.json returns the same "eslint.json: line 2, col 10, Error - Unexpected token :" error.

.js files work great.

a-laughlin commented Jan 6, 2014

Writing this here since the error is identical. Win 7. Git bash 1.8.0. ESLint 0.2.0.

I had the problem with 0.2.0 not parsing .eslintrc. Switching the filename to eslint.json fixed that. eslint now parses the config correctly. However, the same problem is happening with all other .json files. For example, in node_modules/eslint/conf/ , running eslint eslint.json returns the same "eslint.json: line 2, col 10, Error - Unexpected token :" error.

.js files work great.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 6, 2014

Member

It looks like you're trying to parse a JSON file using ESLint, which will
result in that error (JSON is not fully valid JavaScript).

Member

nzakas commented Jan 6, 2014

It looks like you're trying to parse a JSON file using ESLint, which will
result in that error (JSON is not fully valid JavaScript).

@a-laughlin

This comment has been minimized.

Show comment
Hide comment
@a-laughlin

a-laughlin Jan 6, 2014

I assumed that ESLint would lint JSON since JSHint 2.4.1 does. Is JSON linting support intended? I don't see it in the google doc or roadmap.

a-laughlin commented Jan 6, 2014

I assumed that ESLint would lint JSON since JSHint 2.4.1 does. Is JSON linting support intended? I don't see it in the google doc or roadmap.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jan 6, 2014

Member

They seem like unrelated goals to me. IMO, eslint should focus solely on ECMAScript linting, as the name suggests.

Member

michaelficarra commented Jan 6, 2014

They seem like unrelated goals to me. IMO, eslint should focus solely on ECMAScript linting, as the name suggests.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 6, 2014

Contributor

I have always wondered on what basis developers form assumptions like this.

Contributor

aparajita commented Jan 6, 2014

I have always wondered on what basis developers form assumptions like this.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jan 6, 2014

Member

Me, too! This is not uncommon at all. In my experience, it seems not to be a natural phenomenon, but a social one. Like a meme.

Member

michaelficarra commented Jan 6, 2014

Me, too! This is not uncommon at all. In my experience, it seems not to be a natural phenomenon, but a social one. Like a meme.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 6, 2014

Contributor

Here's a possibility: the README says:

"In many ways, it is similar to JSLint and JSHint with a few exceptions..."

The inability to lint JSON was not mentioned as an exception, and the fact that it says "is similar to" vs. "is identical to" is glossed over. So the developer gets it in his mind that ESLint tracks JSHint exactly with a few small exceptions, of which parsing JSON is not one.

Contributor

aparajita commented Jan 6, 2014

Here's a possibility: the README says:

"In many ways, it is similar to JSLint and JSHint with a few exceptions..."

The inability to lint JSON was not mentioned as an exception, and the fact that it says "is similar to" vs. "is identical to" is glossed over. So the developer gets it in his mind that ESLint tracks JSHint exactly with a few small exceptions, of which parsing JSON is not one.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 7, 2014

Member

Sorry if that was unclear. There are no plans to support JSON linting in
ESLint.

Member

nzakas commented Jan 7, 2014

Sorry if that was unclear. There are no plans to support JSON linting in
ESLint.

@a-laughlin

This comment has been minimized.

Show comment
Hide comment
@a-laughlin

a-laughlin Jan 7, 2014

Makes sense. Thanks.

The line I based that assumption on was in v0.1 on the roadmap: "Feature
parity with JSHint - each warning that JSHint outputs must have a
representative warning in ESLint."
On Jan 6, 2014 8:03 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

Sorry if that was unclear. There are no plans to support JSON linting in
ESLint.


Reply to this email directly or view it on GitHubhttps://github.com//issues/486#issuecomment-31704366
.

a-laughlin commented Jan 7, 2014

Makes sense. Thanks.

The line I based that assumption on was in v0.1 on the roadmap: "Feature
parity with JSHint - each warning that JSHint outputs must have a
representative warning in ESLint."
On Jan 6, 2014 8:03 PM, "Nicholas C. Zakas" notifications@github.com
wrote:

Sorry if that was unclear. There are no plans to support JSON linting in
ESLint.


Reply to this email directly or view it on GitHubhttps://github.com//issues/486#issuecomment-31704366
.

@aparajita

This comment has been minimized.

Show comment
Hide comment
@aparajita

aparajita Jan 14, 2014

Contributor

@roadhump Any progress on this?

Contributor

aparajita commented Jan 14, 2014

@roadhump Any progress on this?

nzakas added a commit that referenced this issue Jan 19, 2014

Merge pull request #535 from aparajita/parse-json
Allow config files to have any name (fixes #486).

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.