palantir / tslint

:vertical_traffic_light: An extensible linter for the TypeScript language
http://palantir.github.io/tslint/
Apache License 2.0
5.91k stars 888 forks source link

object-literal-sort-keys is nearly (but not quite) alphabetic #1924

Closed bencoveney closed 7 years ago

bencoveney commented 7 years ago

Bug (Suggestion?) Report

TypeScript code being linted

const expected = {
  "Chiefs": "#3996D9",
  "CLG": "#008FEA",
  "CLG Red": "#DC0031",
  "Cloud9": "#1D9DD8",
  "compLexity": "#870000",
  "Conquest": "#D08685",
  "CPH Wolves": "#FCBA2F",
  "Crowns": "#E3DB9A",
  "CSGL": "#DB8328",
  "CyberZen": "#BC3728",
}

with tslint.json configuration:

{
  "extends": "tslint:recommended",
  "rules": {
    "no-console": false
  }
}

Actual behavior

The following ordering is asserted by the rule:

const actual = {
  "CLG": "#008FEA",
  "CLG Red": "#DC0031",
  "CPH Wolves": "#FCBA2F",
  "CSGL": "#DB8328",
  "Chiefs": "#3996D9",
  "Cloud9": "#1D9DD8",
  "Conquest": "#D08685",
  "Crowns": "#E3DB9A",
  "CyberZen": "#BC3728",
  "compLexity": "#870000",
}

Expected behavior

For context I have an object literal that I am using as a data store where the keys are CSGO team names. The code above is a sample but the real object is about 100 entries. Storing data in literals like this is not a great idea admittedly but the implication is that the keys can contain pretty much any character (for example capital letters) anywhere in the string.

The actual code sample is the order specified by TSLint. It looks like the rule compares strings by just doing > and JavaScript sorts capitals first IIRC which is what is giving the "almost" alphabetical result.

Another simpler example would be:

// What the rule wants
const actualAgain = {
  array: [],
  objList: [{}, {}],
  object: {},
}

// Alphabetical
const expectedAgain = {
  array: [],
  object: {},
  objList: [{}, {}],
}

The result is kind of logical but unexpected. If I were going through the code above and sorting my literals I would do it alphabetically and the fact that the rule told me otherwise would be confusing, it almost reduces you to trial and error unless you understand the comparison logic being used.

So my question is

Also thinking now about the iffy sample I've given, would it be good to subject object literal keys to the same kind of checks performed on identifiers? That is probably a different issue though.

nchen63 commented 7 years ago

Yeah, I don't think it should consider case when sorting

adidahiya commented 7 years ago

Should be pretty easy to change the behavior to be case-insensitive. Change this line to:

const key = keyNode.text.toLowerCase();

... but this would unfortunately be a breaking change. Best thing would be to add this as a "ignore-case" rule option and change the default in v5.0