infctr / eslint-plugin-typescript-sort-keys

A linter plugin to require sorting interface and string enum keys
ISC License
144 stars 28 forks source link

add requiredFirst option #13

Closed nhannah closed 4 years ago

nhannah commented 4 years ago

This is a solution for allowing preference to be given to non-optionals. This option will basically split the type in 2 for sorting with non-optionals first in the list. Required types first is a standard quite often it seems so with sorting it allows for the best of both worlds.

infctr commented 4 years ago

Thank you for the PR!

Unfortunately there seem to be no tests for the "requiredFirst: false" option. Also please add some test cases for the autofix

nhannah commented 4 years ago

Thank you for the PR!

Unfortunately there seem to be no tests for the "requiredFirst: false" option. Also please add some test cases for the autofix

Hey, so I have made the tests but I also found an issue I am not 100% on in the current code:

type Type1<TKey extends string> = Partial<{
  // %foo
  foo: boolean;
  /* %baz */ baz: boolean;

  /**
   * %bar
   */
  bar: boolean;
  /* %bam */ bam: boolean;
}> & {/* %foo */
  foo: boolean;

// %baz
  baz: boolean;
  /**
   * %bar
   */
  bar?: boolean;
} & {
    [K in keyof TKey]: boolean;
  };

If you add bam to the bottom of this type you will get the error Fix objects must not be overlapped in a report. I'll push the new tests and the new option should function as the old plugin does with the addition, but I haven't fixed that potential error in either the new or old code. It seems to be tied to the way comments are autofixed; but I did not dig into it more.

infctr commented 4 years ago

@nhannah Thanks for updating tests! I'll investigate fixer failure later, seems like an edge case.

The PR looks good to me, I'll merge and publish it shortly. I've been working on a complete Typescript rewrite in the meantime which is almost finished, so I'll have to incorporate your changes smh 😄