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

Add object literal sort option to account for special characters #4193

Merged
merged 4 commits into from
Dec 13, 2018
Merged

Add object literal sort option to account for special characters #4193

merged 4 commits into from
Dec 13, 2018

Conversation

pe8ter
Copy link
Contributor

@pe8ter pe8ter commented Sep 28, 2018

PR checklist

Overview of change:

The current behavior of the object-literal-sort-keys rule is to sort any accented characters after the non-accented "z" character, which is undesirable in accent-heavy languages like Spanish, French, and German. This change gives users of this rule the option to sort accented characters next to their non-accented counterpart.

Is there anything you'd like reviewers to focus on?

  • This PR makes no attempt to allow the user to configure which language they want to target. The default behavior of the localeCompare() function is to make a "best fit" guess at which locale to use.
  • The bulk of this PR is auto-formatting changes so I noted the logic change explicitly with a comment.
  • I wasn't sure how to format the CHANGELOG.md message.

Reference links:

CHANGELOG.md entry:

[new-rule-option] object-literal-sort-keys: locale-compare

) {
const key = ignoreCase
? property.name.text.toLowerCase()
: property.name.text;
// comparison with undefined is expected
Copy link
Contributor Author

@pe8ter pe8ter Sep 28, 2018

Choose a reason for hiding this comment

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

Here is where the logic change starts for this PR. The preview doesn't really frame the changes well. The actual change I want to highlight is below this comment.

@pe8ter
Copy link
Contributor Author

pe8ter commented Nov 7, 2018

I'm not sure what's happening. I resolved the conflicts using GitHub's built-in tool and committed my changes, but this page still shows that there's a merge conflict and the commit doesn't appear in this PR's history. If I try to resolve the conflicts again, the tool refuses to save my changes due to an unspecified error. GitHub officially reported a service disruption.

@pe8ter
Copy link
Contributor Author

pe8ter commented Nov 7, 2018

I'm not sure what's happening. GitHub seems to be having issues right now. I fixed my conflicts, but they're not appearing in this PR. I already tried adding a comment stating this, but the comment disappeared mysteriously. GitHub officially reported a service disruption.

Copy link
Member

@ericanderson ericanderson left a comment

Choose a reason for hiding this comment

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

Please update the tests to have something that would fail against master but work in your branch.

Also please rename your directory under test from local-compare to locale-compare

@ericanderson
Copy link
Member

Apologies for the delay in getting to this PR.

@pe8ter
Copy link
Contributor Author

pe8ter commented Dec 12, 2018

Thank you for your feedback. I made all requested changes:

  • Use data set where tests fail in master, but pass in feature branch.
  • Rename local-comparelocale-compare.

@ericanderson ericanderson merged commit a4ff185 into palantir:master Dec 13, 2018
@pe8ter pe8ter deleted the local-sort branch December 13, 2018 01:13
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.

3 participants