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

[New] order: add caseInsensitive: 'invert' option #1740

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

forivall
Copy link
Contributor

Adds the "invert" option to "caseInsensitive"

  • caseInsensitive: use true to ignore case, false to consider case, and 'invert' to sort lowercase before uppercase (default: false).

bikeshed: I'd rename caseInsensitive to caseSensitive with default true. other naming options are viable.

@forivall
Copy link
Contributor Author

My current workaround which works for ~80% of the instances in the target codebase is

const rules = {
  ...
  'import/order': [
    'warn',
    {
      groups: ['builtin', 'external', 'internal', 'parent', 'sibling', 'index'],
      alphabetize: {
        order: 'asc',
        caseInsensitive: true,
      },
      pathGroups: [
        { pattern: './[A-Z]*{,/**}', group: 'sibling', position: 'after' },
      ],
    },
  ],
  ...
}

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 97.746% when pulling 6013f1b on forivall:feat/case-sort-invert into 92caa35 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 97.746% when pulling 6013f1b on forivall:feat/case-sort-invert into 92caa35 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 97.746% when pulling 6013f1b on forivall:feat/case-sort-invert into 92caa35 on benmosher:master.

@coveralls
Copy link

coveralls commented Apr 29, 2020

Coverage Status

Coverage increased (+2.01%) to 71.516% when pulling 3e1d204 on forivall:feat/case-sort-invert into e871a9a on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

The code seems fine to me. Can you elaborate on the use case?

@ljharb ljharb force-pushed the feat/case-sort-invert branch from 6013f1b to bc8390f Compare June 2, 2020 20:49
@forivall
Copy link
Contributor Author

forivall commented Jul 7, 2020

Sure: the use case is that in the frontend code at $job, data/core modules start with a lower case, and the Components start with UpperCase, and so, when i'm sorting imports, i want the business logic imports to be first, and then the imports of Presentation Logic.

(sorry for the late reply, got lost in my notifications)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM overall, just a few comments

@@ -211,12 +211,12 @@ import index from './';
import sibling from './foo';
```

### `alphabetize: {order: asc|desc|ignore, caseInsensitive: true|false}`:
### `alphabetize: {order: asc|desc|ignore, caseInsensitive: true|false|'invert'}`:
Copy link
Member

Choose a reason for hiding this comment

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

bikeshed: maybe in addition to true/false, we could support (and recommend) a three-option string enum? iow, this can be 'ignore' | 'upperFirst' | 'lowerFirst', with false being an alias for ignore and true being an alias for upperFirst?

Copy link
Contributor Author

@forivall forivall Mar 3, 2021

Choose a reason for hiding this comment

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

Hmm, perhaps the "caseInsensitive" should be deprecated and make the 3 option string enum under an option called "caseSensitive" or "caseSensitivity"?

i wonder what other things call their options?

With that all said, i'd like to suggest keeping caseInsensitive as a boolean, and for this option, introduce caseFirst: 'upper' | 'lower', based on the Intl.Collator options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably also be a good idea to just use Intl.Collator like what typescript-eslint does instead of doing the case-munging manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alternative diff, using caseFirst is here: master...forivall:feat/case-first

Comment on lines 288 to 291
const sorterFn = getSorter(alphabetizeOptions.order === 'asc')
const comparator = alphabetizeOptions.caseInsensitive ? (a, b) => sorterFn(String(a).toLowerCase(), String(b).toLowerCase()) : (a, b) => sorterFn(a, b)
const comparator =
alphabetizeOptions.caseInsensitive === 'invert' ? (a, b) => sorterFn(swapCase(String(a)), swapCase(String(b)))
: alphabetizeOptions.caseInsensitive ? (a, b) => sorterFn(String(a).toLowerCase(), String(b).toLowerCase())
Copy link
Member

Choose a reason for hiding this comment

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

seems like this should be handled inside getSorter rather than here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved with my new refactor commit.

@ljharb ljharb added the accepted label Feb 1, 2021
@ljharb ljharb marked this pull request as draft February 1, 2021 18:52
@forivall forivall force-pushed the feat/case-sort-invert branch 2 times, most recently from 5b68e4f to 3e1d204 Compare March 3, 2021 03:13
@forivall forivall force-pushed the feat/case-sort-invert branch from 3e1d204 to f8eabbd Compare March 3, 2021 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants