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

fix: sort-keys spreads, spreads with generics, comment handling #439

Merged

Conversation

sompylasar
Copy link
Contributor

For the flowtype/sort-keys rule:

  1. Fixes dropping of spread generic type arguments when keys are reordered.
  2. Fixes reordering of spread elements. The order of spreads is significant and must be preserved, sort only standalone keys between the spans of spreads.
  3. Fixes comment handling and preservation.
  4. Makes it clear in the tests and code comments that the objects within generic type arguments are not auto-fixed even though they show an error (can be fixed later).

I had to reimplement generateOrderedList and a part of generateFix to get this working to find tokens an replacement ranges more precisely (most difficult were comments). Hope it won't be very hard to review; I recommend reviewing the full code, not the red/green diff lines.

See added tests. Before this PR, the added tests were failing:

  1419 passing (2s)
  5 failing

  1) sort-keys
       invalid
         
        type FooType = {
          ...ErrorsInRecursiveGenericTypeArgsButDoesNotFix<{
            y: boolean,
            x: string,
            z: {
              j: string,
              l: number,
              k: boolean,
            },
          }>,
          a: number,
          c: string,
          b: Map<string, Array<ErrorsInRecursiveGenericTypeArgsButDoesNotFix<{
            y: boolean,
            x: string,
            z: {
              j: string,
              l: number,
              k: boolean,
            },
          }>>>,
        }
      :

      AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
      + expected - actual

       
               type FooType = {
      -          ...ErrorsInRecursiveGenericTypeArgsButDoesNotFix,
      +          ...ErrorsInRecursiveGenericTypeArgsButDoesNotFix<{
      +            y: boolean,
      +            x: string,
      +            z: {
      +              j: string,
      +              l: number,
      +              k: boolean,
      +            },
      +          }>,
                 a: number,
                 b: Map<string, Array<ErrorsInRecursiveGenericTypeArgsButDoesNotFix<{
                   y: boolean,
                   x: string,
      
      at testInvalidTemplate (node_modules/eslint/lib/testers/rule-tester.js:569:28)
      at Context.<anonymous> (node_modules/eslint/lib/testers/rule-tester.js:592:25)
      at processImmediate (internal/timers.js:439:21)

  2) sort-keys
       invalid
         
        type FooType = {
          ...BPreservesSpreadOrder,
          ...APreservesSpreadOrder,
          c: string,
          b: number,
        }
      :

      AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
      + expected - actual

       
               type FooType = {
      +          ...BPreservesSpreadOrder,
                 ...APreservesSpreadOrder,
      -          ...BPreservesSpreadOrder,
                 b: number,
                 c: string,
               }
             
      
      at testInvalidTemplate (node_modules/eslint/lib/testers/rule-tester.js:569:28)
      at Context.<anonymous> (node_modules/eslint/lib/testers/rule-tester.js:592:25)
      at processImmediate (internal/timers.js:439:21)

  3) sort-keys
       invalid
         
        type FooType = {
          ...BPreservesSpreadSpans,
          ...APreservesSpreadSpans,
          c: string,
          b: number,
          ...CPreservesSpreadSpans,
          e: string,
          d: number,
        }
      :

      AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
      + expected - actual

       
               type FooType = {
      +          ...BPreservesSpreadSpans,
                 ...APreservesSpreadSpans,
      -          ...BPreservesSpreadSpans,
      -          ...CPreservesSpreadSpans,
                 b: number,
                 c: string,
      +          ...CPreservesSpreadSpans,
                 d: number,
                 e: string,
               }
             
      
      at testInvalidTemplate (node_modules/eslint/lib/testers/rule-tester.js:569:28)
      at Context.<anonymous> (node_modules/eslint/lib/testers/rule-tester.js:592:25)
      at processImmediate (internal/timers.js:439:21)

  4) sort-keys
       invalid
         
        type FooType = {
          ...BPreservesSpreadOrderAndTypeArgs<string, number>,
          ...APreservesSpreadOrderAndTypeArgs<number>,
          c: string,
          b: number,
        }
      :

      AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
      + expected - actual

       
               type FooType = {
      -          ...APreservesSpreadOrderAndTypeArgs,
      -          ...BPreservesSpreadOrderAndTypeArgs,
      +          ...BPreservesSpreadOrderAndTypeArgs<string, number>,
      +          ...APreservesSpreadOrderAndTypeArgs<number>,
                 b: number,
                 c: string,
               }
             
      
      at testInvalidTemplate (node_modules/eslint/lib/testers/rule-tester.js:569:28)
      at Context.<anonymous> (node_modules/eslint/lib/testers/rule-tester.js:592:25)
      at processImmediate (internal/timers.js:439:21)

  5) sort-keys
       invalid
         
        type FooType = {
          /* preserves block comment before spread BType */
          // preserves line comment before spread BType
          ... /* preserves comment in spread BType */ BType<Generic> /* preserves trailing comment in spread AType */,
          /* preserves block comment before spread AType */
          // preserves line comment before spread AType
          ... /* preserves comment in spread AType */ AType /* preserves trailing comment in spread AType */,
          /* preserves block comment before reordered key "c" */
          // preserves line comment before reordered key "c"
          c:/* preserves comment and white space or lack of it */string/* preserves trailing comment for key "c" */,
          b: number,
          dWithoutComma: boolean
        }
      :

      AssertionError [ERR_ASSERTION] [ERR_ASSERTION]: Output is incorrect.
      + expected - actual

       
               type FooType = {
                 /* preserves block comment before spread BType */
                 // preserves line comment before spread BType
      -          ...AType /* preserves trailing comment in spread AType */,
      +          ... /* preserves comment in spread BType */ BType<Generic> /* preserves trailing comment in spread AType */,
                 /* preserves block comment before spread AType */
                 // preserves line comment before spread AType
      -          ...BType /* preserves trailing comment in spread AType */,
      +          ... /* preserves comment in spread AType */ AType /* preserves trailing comment in spread AType */,
      +          b: number,
                 /* preserves block comment before reordered key "c" */
                 // preserves line comment before reordered key "c"
      -          b: number/* preserves trailing comment for key "c" */,
      -          c: string,
      +          c:/* preserves comment and white space or lack of it */string/* preserves trailing comment for key "c" */,
                 dWithoutComma: boolean
               }
             
      
      at testInvalidTemplate (node_modules/eslint/lib/testers/rule-tester.js:569:28)
      at Context.<anonymous> (node_modules/eslint/lib/testers/rule-tester.js:592:25)
      at processImmediate (internal/timers.js:439:21)

@gajus gajus merged commit dccaa76 into gajus:master Jan 1, 2020
@gajus
Copy link
Owner

gajus commented Jan 1, 2020

🎉 This PR is included in version 4.5.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants