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

member-ordering: Add 'alphabetize' option #2101

Merged
merged 2 commits into from Jan 30, 2017

Conversation

andy-hanson
Copy link
Contributor

PR checklist

What changes did you make?

Added the 'alphabetize' option to member-ordering, which enforces alphabetical ordering within each category.

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

This uses < for string comparison. I think .localeCompare would be troublesome if people in different locales want linting to succeed on all of their machines.

}

const curName = nameString(member.name);
if (prevName !== undefined && curName < prevName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think < takes case into account, but I don't think it should

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, in ordered-imports, we use < and >, but it calls out case-insensitive, lowercase-first and lowercase-last. I think we could just default to case insensitive and call .toLowerCase() before comparison

@nchen63
Copy link
Contributor

nchen63 commented Jan 26, 2017

@andy-hanson please rebase

@nchen63 nchen63 merged commit d7c2bde into palantir:master Jan 30, 2017
@nchen63
Copy link
Contributor

nchen63 commented Jan 30, 2017

@andy-hanson thanks!

@ackvf
Copy link

ackvf commented Apr 26, 2018

How to use the option? It's not apparent from the documentation.

@mmakarios
Copy link

@ackvf Pass the 'alphabetize' option to the options object of the member-ordering rule.

Example:

"member-ordering": [
  true,
  {
    "order": [
      "static-field",
      "instance-field",
      "static-method",
      "instance-method"
    ],
    "alphabetize": true
  }
],

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

Successfully merging this pull request may close these issues.

None yet

4 participants