Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Fixed object-literal-sort-keys rule ignores quoted keys #1512 #1516

Closed
wants to merge 3 commits into from
Closed

Fixed object-literal-sort-keys rule ignores quoted keys #1512 #1516

wants to merge 3 commits into from

Conversation

bolatovumar
Copy link
Contributor

No description provided.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @bolatovumar! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@@ -62,7 +62,7 @@ class ObjectLiteralSortKeysWalker extends Lint.RuleWalker {
if (sortedState) {
const lastSortedKey = this.lastSortedKeyStack[this.lastSortedKeyStack.length - 1];
const keyNode = node.name;
if (keyNode.kind === ts.SyntaxKind.Identifier) {
if (keyNode.kind === ts.SyntaxKind.Identifier || keyNode.kind === ts.SyntaxKind.StringLiteral) {
const key = (<ts.Identifier> keyNode).text;
Copy link
Contributor

Choose a reason for hiding this comment

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

this type assertion is no longer accurate with the change in the line above. I suggest refactoring into a type guard

function isIdentifierOrStringLiteral(node: ts.Node): node is (ts.Identifier | ts.StringLiteral) {
    return node.kind === ts.SyntaxKind.Identifier || node.kind === ts.SyntaxKind.StringLiteral;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@bolatovumar
Copy link
Contributor Author

@adidahiya Fixing this issue breaks the build as many files in the source code have quoted keys which are not in alphabetical order. What would the ideal resolution be here? Should I position them alphabetically and create a separate pull request? Or submit everything as one pull request? Something else entirely?

@jkillian
Copy link
Contributor

Doing it as a separate PR sounds good to me 👍

@bolatovumar
Copy link
Contributor Author

Issue a new pull request here

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

Successfully merging this pull request may close these issues.

4 participants