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

tools: add quote-props eslint rule #14059

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 3, 2017

This adds the quote-props rule and uses the consistent style. I personally would like to change that to consistent-as-needed but this would create quite some churn and therefore using consistent right now is probably best.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark,test,tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 3, 2017
@tniessen
Copy link
Member

tniessen commented Jul 3, 2017

@BridgeAR BridgeAR changed the title eslint: add quote props tools: add quote-props eslint rule Jul 3, 2017
tniessen
tniessen previously approved these changes Jul 3, 2017
@tniessen tniessen self-assigned this Jul 3, 2017
@refack
Copy link
Contributor

refack commented Jul 3, 2017

I personally would like to change that to consistent-as-needed but this would create quite some churn and therefore using consistent right now is probably best.

Personally I'd rather have as-needed if we're going to churn (392 problems in 71 files)
(for reference consistent-as-needed has 317 problems in 55 files)

@refack
Copy link
Contributor

refack commented Jul 3, 2017

I think I'm -1 on this.
I'm not blocking but would like a discussion. Is consistently better than "parsimony"?

@tniessen
Copy link
Member

tniessen commented Jul 3, 2017

I think this is a design decision, do we prefer consistent style or do we want to leave out all quotation marks that are not strictly required?

@tniessen tniessen dismissed their stale review July 3, 2017 16:54

Ongoing design decision - do not land!

@fhinkel fhinkel added the discuss Issues opened for discussions and feedbacks. label Jul 3, 2017
@lpinca
Copy link
Member

lpinca commented Jul 4, 2017

-0, I prefer to not use quotes unless it is really needed.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 4, 2017

I don't mind using as-needed. I think that most people prefer not to use quotes, that's the main point about this PR.
Using consistent is meant as a small step towards that since most objects don't need quotes at all. I tried not to churn to much and now things could be changed over time or is churning justified and I should just change this to either consistent-as-needed or to as-needed?

@refack
Copy link
Contributor

refack commented Jul 4, 2017

I don't mind using as-needed. I think that most people prefer not to use quotes, that's the main point about this PR.
Using consistent is meant as a small step towards that since most objects don't need quotes at all. I tried not to churn to much and now things could be changed over time or is churning justified and I should just change this to either consistent-as-needed or to as-needed?

I think we have precedence for churning in order to activate an eslint rule ( @vsemozhetbyt is the king of those 👑 #12563, #13835, etc. )

IMHO as-needed is best, but up for a quick vote:

  •    ❤️     — do nothing
  • 👍  — consistent (current state of PR)
  • 😄 — as-needed
  • 🎉 — consistent-as-needed

/cc @nodejs/collaborators

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 4, 2017

I am really not sure what to prefer. I like clear minimization (as-needed), but I recall a discussion about object shorthand: some collaborators said they considered this confusing:

const foo = 'foo';

const obj = {
  foo,
  bar: 'abc'
}

vs

const obj = {
  foo: foo,
  bar: 'abc'
}

as a mind tunes in for on style and then stumbles over a different one and that hampers reading. For me, the first example is OK, but I would not want to torture those who see this differently, i.e. who possibly prefer consistent-as-needed)

@refack
Copy link
Contributor

refack commented Jul 4, 2017

For me, the first example is OK, but I would not want to torture those who see this differently)

Since quote-props only check prop names, I think the better example would be:

const foo = 'foo';

const obj = {
  'foo': foo,
  bar: 'abc'
}

@vsemozhetbyt
Copy link
Contributor

Yes, I meant we already had a discussion on a similar style issue)

@BridgeAR
Copy link
Member Author

Can we get some further opinions here?

@evanlucas
Copy link
Contributor

IMO, we should only quote properties that must be quoted. Rule changes tend to make cherry-picking for releases more difficult...

@mcollina
Copy link
Member

I think this just creates churn for no particular benefit: -1. It also complicates cherry-picking.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 10, 2017

Also -1 for the reasons listed.

@gibfahn
Copy link
Member

gibfahn commented Jul 10, 2017

I'd prefer as-needed, but I'm not sure there's enough benefit here for it to be worthwhile.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also -1 for both the reasons stated above (churn) and stylistic preference. I'm making my -1 explicit - I'm fine with "as needed" as a config or not changing the config.

@refack
Copy link
Contributor

refack commented Jul 10, 2017

@mcollina @cjihrig Just to be clear would you be Ok with as-needed (and fixing 392 problems in 71 files)?

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

I'm -1 on this too. I much prefer omitting the prop name quotes when possible to do so.

@refack
Copy link
Contributor

refack commented Jul 10, 2017

@mcollina @cjihrig Just to be clear would you be Ok with as-needed (and fixing 392 problems in 71 files)?

Also we could set quote-props: [warning, as-needed] for a transition period

@mcollina
Copy link
Member

I'm -1 as as-needed and as well.

@refack
Copy link
Contributor

refack commented Jul 10, 2017

Closing for lack of consensus

@refack refack closed this Jul 10, 2017
@BridgeAR BridgeAR added the invalid Issues and PRs that are invalid. label Sep 14, 2017
@BridgeAR BridgeAR deleted the add-quote-props branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. invalid Issues and PRs that are invalid. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.