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

Proposal: minimum number of keys option for sort-keys #11624

Closed
csantos42 opened this issue Apr 16, 2019 · 4 comments · Fixed by #11625 · May be fixed by Omrisnyk/npm-lockfiles#130
Closed

Proposal: minimum number of keys option for sort-keys #11624

csantos42 opened this issue Apr 16, 2019 · 4 comments · Fixed by #11625 · May be fixed by Omrisnyk/npm-lockfiles#130
Assignees
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@csantos42
Copy link
Contributor

Background

My team is undergoing an exercise where we re-evaluate our enabled lint rules to identify any pain points.

The most unpopular so far is sort-keys, as the value of this rule is only apparent for object literals with a large number of keys (the extreme case being an object containing something like the dependency list in package.json, where it's extremely useful that the keys are sorted).

For objects containing 2 or 3 keys, it becomes more of a chore, especially considering the fact that this rule is not auto-fixable by design.


What rule do you want to change?
sort-keys

Does this change cause the rule to produce more or fewer warnings?
Fewer when the option is set as it would allow the user to provide a minimum number of keys that an object literal should have in order for this rule to take effect. By default, the minimum number of keys would be 2.

How will the change be implemented? (New option, new default behavior, etc.)?
New option -- I'm thinking minKeys is consistent with the naming of other options throughout ESLint, but happy to hear other suggestions.

Please provide some example code that this change will affect:

/* eslint sort-keys: ["error", { minKeys: 5 }] */

const styles = {
  position: 'relative',
  color: 'blue'
};

const styles2 = {
  fontSize: 16,
  position: 'relative',
  color: 'blue',
  zIndex: 10,
  display: 'block',
};

What does the rule currently do for this code?
ESLint produces 3 errors, one error for the property ordering of styles and two errors for the property ordering of styles2.

What will the rule do after it's changed?
With minKeys set to 5, only styles2 will produce lint errors, so there will be a total of 2 lint errors.

Are you willing to submit a pull request to implement this change?
Yes, happy to open a PR if the proposal reaches consensus

@csantos42 csantos42 added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Apr 16, 2019
@platinumazure
Copy link
Member

This seems reasonable. I'll support. 👍

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 17, 2019
csantos42 added a commit to csantos42/eslint that referenced this issue Apr 17, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label May 18, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@dcastil
Copy link

dcastil commented May 27, 2019

I'd like to have this implemented, too! 😊 Waiting for the feature to be merged!

@kaicataldo
Copy link
Member

I'll champion this. I think this is worth another look, given the large support from the team (3 thumbs up here and 1 on the corresponding PR) and the existence of the PR.

@kaicataldo kaicataldo self-assigned this May 29, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed auto closed The bot closed this issue evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 29, 2019
@kaicataldo kaicataldo reopened this May 29, 2019
platinumazure pushed a commit that referenced this issue Jun 8, 2019
…1625)

* Fix #11624 by adding minKeys option to sort-keys

* Update documentation for sort-keys with minKeys option
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 6, 2019
@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 Dec 6, 2019
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
4 participants