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

no-reserved-keys and quote-props have incompatibilities. #1539

Closed
terinjokes opened this issue Dec 2, 2014 · 13 comments
Closed

no-reserved-keys and quote-props have incompatibilities. #1539

terinjokes opened this issue Dec 2, 2014 · 13 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@terinjokes
Copy link

The following Identifiers give warnings from no-reserved-keys when not quoted, but also give warnings from quote-props when set in "asNeeded" mode.

abstract
boolean
byte
char
double
final
float
goto
implements
int
interface
long
native
package
private
protected
public
short
static
synchronized
throws
transient
volatile

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@nzakas
Copy link
Member

nzakas commented Dec 2, 2014

Can you include some actual examples and the output you're seeing?

@nzakas nzakas added triage An ESLint team member will look at this issue soon rule Relates to ESLint's core rules labels Dec 2, 2014
@terinjokes
Copy link
Author

var minimist = require('minimist');
var args = minimist(process.argv.slice(2), {
  string: 'env',
  boolean: ['sourcemaps', 'minify'],
  'default': {
    env: 'development'
  }
});

If "boolean" is unqouted:

2:43 error  Reserved word 'boolean' used as key  no-reserved-keys

If "boolean" is quoted:

4:2  warning  Unnecessarily quoted property `{{name}}` found  quote-props

@nkbt
Copy link

nkbt commented Apr 8, 2015

Using CSS 'as JS' causes this problem.

const imgStyle = {
  'float': 'right'
};

// error  Unnecessarily quoted property `float` found  quote-props
const imgStyle = {
  float: 'right'
};

// error  Reserved word 'float' used as key  no-reserved-keys

@ilyavolodin ilyavolodin added bug ESLint is working incorrectly accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 23, 2015
@jrvidal
Copy link
Contributor

jrvidal commented Jun 17, 2015

Would it make sense to merge the two rules? We could keep quote-props with an additional option such as "as-needed-es3".

@nzakas
Copy link
Member

nzakas commented Jun 17, 2015

We'd need to work on the option name, but it would seem helpful to standardize on just quote-props

@jrvidal
Copy link
Contributor

jrvidal commented Jun 17, 2015

OK, I'll work on a PR.

@nzakas
Copy link
Member

nzakas commented Jun 18, 2015

Can you discuss what you're planning on doing first? Make it easier for people to chime in (and less thrash on the PR)

@jrvidal
Copy link
Contributor

jrvidal commented Jun 18, 2015

Hum, I was planning to merge the two rules into quote-props, I'm not sure if that what you're asking. That means:

  • Mark no-reserved-keys as deprecated.
  • Import its keyword list to quote-props and use it for as-needed-es3 (working name).

@nzakas
Copy link
Member

nzakas commented Jun 18, 2015

We've had some issues with keyword lists. See: #1616

I'm not sure if as-needed-es3 is clear enough or if there needs to be something more (or ability to specify which words to enforce).

@jrvidal
Copy link
Contributor

jrvidal commented Jun 29, 2015

As far as I can see, the only rules that deal with explicit keyword lists are no-reserved-keys (quote-props after the merge) and dot-notation. I think we could refactor them to use a common keyword list, broken down in ES3/ES5 categories. We could tackle #1616 later by refining that list. Does it make sense?

BTW, I couldn't find any examples of a common resource used by several rules, apart from the context API. In other words, would this approach make sense?

/**
 * Rule X
 */

var keywords = require("../utils/keywords");

// ...

// cc @nzakas

@nzakas
Copy link
Member

nzakas commented Jun 30, 2015

We've debated how to share amongst rules, and I think at some point we just need to go for it and see what happens, so that approach is fine by me.

I think what we really need for this rule is a configuration like:

quote-props: [2, "as-needed", { keywords: true }

@jrvidal
Copy link
Contributor

jrvidal commented Jul 1, 2015

A few odds and ends I found:

  • Numbers as properties:

    // (1a) with "as-needed"
    var x = ({100: 1});
    
    // (1b) with "as-needed"
    var x = ({1e2: 1});
    
    // (1c) with "as-needed"
    var x = ({'100': 1});
    
    // (1d) with "as-needed"
    var x = ({'1e2': 1});

    With as-needed, (1c) throws, all the others pass. I think the user should be able to configure this behavior as well. I'm thinking of an additional numbers option:

    • numbers: "always": (1c) and (1d) pass
    • numbers: "ambigous": (1a) and (1d) pass
    • numbers: "never": (1a) and (1b) pass
  • Special properties:
    Right now the rule interacts in funny ways with all "special properties":

    // (2a) with "always" and ecmaFeatures
    var x = {[foo]: 1};
    
    // (2b) with "always" and ecmaFeatures
    var x = {foo,};
    
    // (2c) with "as-needed"
    var x = {get "foo"() {}}
    
    // (2d) with "as-needed" and ecmaFeatures
    var x = {"foo"() {}}
    
    // (2e) with "as-needed" and ecmaFeatures
    var x = {["foo"]: 1}

    All these throw. I propose to ignore all "special" properties for now. We can discuss later whether to include them in this rule or to create a new one.

@nzakas
Copy link
Member

nzakas commented Jul 1, 2015

@jrvidal please open a separate issue for those and include the actual ESLintb output. This topic is too tangential for this issue.

jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 3, 2015
jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 3, 2015
jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 3, 2015
jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 14, 2015
jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 23, 2015
jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 23, 2015
jrvidal added a commit to jrvidal/eslint that referenced this issue Jul 24, 2015
@nzakas nzakas closed this as completed in 522dc89 Jul 31, 2015
nzakas added a commit that referenced this issue Jul 31, 2015
Update: merge `no-reserved-keys` into `quote-props` (fixes #1539)
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

5 participants