Merge pull request #13 from nhannah/add-required-first-option

add requiredFirst option

Author: infctr

docs/rules/interface.md

@@ -36,6 +36,13 @@ interface U {
   10: T;
 }
 
+// Non-required first order by default.
+interface U {
+  b?: T;
+  a: T;
+  c: T;
+}
+
 interface U {
   a: T;
   ['c']: T;
@@ -73,6 +80,13 @@ interface U {
   2: T;
 }
 
+// Non-required first order by default.
+interface U {
+  a: T;
+  b?: T;
+  c: T;
+}
+
 // This rule checks computed properties which have a simple name as well.
 interface U {
   a: T;
@@ -88,7 +102,7 @@ interface U {
   "typescript-sort-keys/interface": [
     "error",
     "asc",
-    { "caseSensitive": true, "natural": false }
+    { "caseSensitive": true, "natural": false, "requiredFirst": false }
   ]
 }
 ```
@@ -98,10 +112,11 @@ The 1st option is `"asc"` or `"desc"`.
 - `"asc"` (default) - enforce properties to be in ascending order.
 - `"desc"` - enforce properties to be in descending order.
 
-The 2nd option is an object which has 2 properties.
+The 2nd option is an object which has 3 properties.
 
 - `caseSensitive` - if `true`, enforce properties to be in case-sensitive order. Default is `true`.
 - `natural` - if `true`, enforce properties to be in natural order. Default is `false`. Natural Order compares strings containing combination of letters and numbers in the way a human being would sort. It basically sorts numerically, instead of sorting alphabetically. So the number 10 comes after the number 3 in Natural Sorting.
+- `requiredFirst` - if `true`, enforce optional properties to come after required ones.
 
 Example for a list:
 
@@ -144,6 +159,13 @@ interface U {
   a: T;
 }
 
+// Non-required first order by default.
+interface U {
+  a: T;
+  b?: T;
+  c: T;
+}
+
 // Non-natural order by default.
 interface U {
   10: T;
@@ -175,6 +197,13 @@ interface U {
   C: T;
 }
 
+// Non-required first order by default.
+interface U {
+  c: T;
+  b?: T;
+  a: T;
+}
+
 // Non-natural order by default.
 interface U {
   2: T;
@@ -249,6 +278,34 @@ interface U {
 }
 ```
 
+### required
+
+Examples of **incorrect** code for the `{ requiredFirst: true }` option:
+
+```ts
+/* eslint typescript-sort-keys/interface: ["error", "asc", { requiredFirst: true }] */
+
+interface U {
+  d: T;
+  c?: T;
+  b?: T;
+  a: T;
+}
+```
+
+Examples of **correct** code for the `{ requiredFirst: true }` option:
+
+```ts
+/* eslint typescript-sort-keys/interface: ["error", "asc", { requiredFirst: true }] */
+
+interface U {
+  a: T;
+  d: T;
+  b?: T;
+  c?: T;
+}
+```
+
 ## When Not To Use It
 
 If you don't want to notify about properties' order, then it's safe to disable this rule.

lib/plugin.js

@@ -1,6 +1,6 @@
 const assert = require('assert');
 
-const { getPropertyName } = require('./utils/ast');
+const { getPropertyName, getPropertyOptional } = require('./utils/ast');
 const { compareFunctions } = require('./utils/compareFunctions');
 
 function createNodeSwapper(context) {
@@ -131,6 +131,7 @@ module.exports = function createReporter(context, createReportObject) {
   const options = context.options[1];
   const insensitive = (options && options.caseSensitive) === false;
   const natural = Boolean(options && options.natural);
+  const requiredFirst = (options && options.requiredFirst) === true;
   const computedOrder = [order, insensitive && 'I', natural && 'N']
     .filter(Boolean)
     .join('');
@@ -139,9 +140,21 @@ module.exports = function createReporter(context, createReportObject) {
   const swapNodes = createNodeSwapper(context);
 
   return body => {
-    const sortedBody = [...body].sort((a, b) => {
-      return compareFn(getPropertyName(a), getPropertyName(b));
-    });
+    const required = [...body]
+      .filter(node => !getPropertyOptional(node))
+      .sort((a, b) => {
+        return compareFn(getPropertyName(a), getPropertyName(b));
+      });
+    const optional = [...body]
+      .filter(node => getPropertyOptional(node))
+      .sort((a, b) => {
+        return compareFn(getPropertyName(a), getPropertyName(b));
+      });
+    const sortedBody = requiredFirst
+      ? [...required, ...optional]
+      : [...body].sort((a, b) => {
+          return compareFn(getPropertyName(a), getPropertyName(b));
+        });
 
     const nodePositions = new Map(
       body.map(n => [
@@ -156,7 +169,15 @@ module.exports = function createReporter(context, createReportObject) {
       const prevNodeName = getPropertyName(prevNode);
       const currentNodeName = getPropertyName(currentNode);
 
-      if (compareFn(prevNodeName, currentNodeName) > 0) {
+      if (
+        (!requiredFirst && compareFn(prevNodeName, currentNodeName) > 0) ||
+        (requiredFirst &&
+          getPropertyOptional(prevNode) === getPropertyOptional(currentNode) &&
+          compareFn(prevNodeName, currentNodeName) > 0) ||
+        (requiredFirst &&
+          getPropertyOptional(prevNode) !== getPropertyOptional(currentNode) &&
+          getPropertyOptional(prevNode))
+      ) {
         const targetPosition = sortedBody.indexOf(currentNode);
         const replaceNode = body[targetPosition];
         const { data, ...rest } = createReportObject(currentNode, replaceNode);
@@ -179,6 +200,7 @@ module.exports = function createReporter(context, createReportObject) {
             order,
             insensitive: insensitive ? 'insensitive ' : '',
             natural: natural ? 'natural ' : '',
+            requiredFirst: requiredFirst ? 'required first ' : '',
             ...data,
           },
           fix: fixer => {

lib/rules/interface.js

@@ -28,6 +28,9 @@ module.exports = {
           natural: {
             type: 'boolean',
           },
+          requiredFirst: {
+            type: 'boolean',
+          },
         },
         additionalProperties: false,
       },
@@ -38,7 +41,7 @@ module.exports = {
     const compareNodeListAndReport = createReporter(context, currentNode => ({
       loc: currentNode.key ? currentNode.key.loc : currentNode.loc,
       message:
-        'Expected interface keys to be in {{ natural }}{{ insensitive }}{{ order }}ending order. ' +
+        'Expected interface keys to be in {{ requiredFirst }}{{ natural }}{{ insensitive }}{{ order }}ending order. ' +
         `'{{ thisName }}' should be before '{{ prevName }}'.`,
     }));
     return {

lib/utils/ast.js

@@ -12,7 +12,7 @@ module.exports = {
   },
 
   /**
-   * Gets the property name of a given node.
+   * Gets the property name and optional value of a given node.
    * The node can be a TSPropertySignature, or a TSMethodSignature.
    *
    * If the name is dynamic, this returns `null`.
@@ -39,10 +39,14 @@ module.exports = {
    *     let a = {[tag`b`]: 1}     // => null
    *     let a = {[`${b}`]: 1}     // => null
    *
+   * @typedef {Object} NameAndOptional
+   * @property {string|undefined} name - Name of node
+   * @property {boolean|undefined} isOptional - Optional value of node
+   *
    * @param {ASTNode} node - The node to get.
-   * @returns {string|null} The property name if static. Otherwise, null.
+   * @returns {NameAndOptional} - The property name and optional value if static.
    */
-  getStaticPropertyName(node) {
+  getStaticPropertyNameAndOptional(node) {
     let prop;
 
     switch (node && node.type) {
@@ -70,24 +74,47 @@ module.exports = {
 
     switch (prop && prop.type) {
       case 'Literal':
-        return String(prop.value);
+        return {
+          name: String(prop.value),
+          isOptional: prop.optional,
+        };
 
       case 'TemplateLiteral':
         if (prop.expressions.length === 0 && prop.quasis.length === 1) {
-          return prop.quasis[0].value.cooked;
+          return {
+            name: prop.quasis[0].value.cooked,
+            isOptional: prop.optional,
+          };
         }
         break;
 
       case 'Identifier':
         if (!node.computed) {
-          return prop.name;
+          return {
+            name: prop.name,
+            isOptional: prop.optional,
+          };
         }
         break;
 
       // no default
     }
 
-    return null;
+    return {};
+  },
+
+  /**
+   *
+   * Gets if the property is optional given `Property` node.
+   *
+   * @param {ASTNode} node - The `Property` node to get.
+   * @returns {boolean} - The optional value or false.
+   */
+  getPropertyOptional(node) {
+    const { isOptional } = module.exports.getStaticPropertyNameAndOptional(
+      node,
+    );
+    return !!(isOptional || node.optional);
   },
 
   /**
@@ -99,14 +126,12 @@ module.exports = {
    * - Otherwise, this returns null.
    *
    * @param {ASTNode} node - The `Property` node to get.
-   * @returns {string|null} The property name or null.
+   * @returns {string|null} - The property name or null.
    * @private
    */
   getPropertyName(node) {
-    return (
-      module.exports.getStaticPropertyName(node) ||
-      (node.key && node.key.name) ||
-      null
-    );
+    const { name } = module.exports.getStaticPropertyNameAndOptional(node);
+
+    return name || (node.key && node.key.name) || null;
   },
 };

tests/autofix.spec.js

@@ -55,3 +55,52 @@ describe('autofix', () => {
     assert.strictEqual(output, expected);
   });
 });
+
+describe('autofix-required-first', () => {
+  it('should properly format comments and indent level for required first option', () => {
+    const { name: tmpDir } = tmp.dirSync({
+      prefix: 'typescript-sort-keys-',
+      unsafeCleanup: true,
+    });
+
+    const testFilePath = Path.join(tmpDir, 'autofix-required-first.ts');
+
+    const input = fs.readFileSync('tests/fixtures/autofix.input.ts', 'utf8');
+
+    const expected = fs.readFileSync(
+      'tests/fixtures/autofix-required-first.output.ts',
+      'utf8',
+    );
+
+    fs.writeFileSync(testFilePath, input);
+
+    const result = spawn.sync(
+      'eslint',
+      [
+        '--ext',
+        '.ts',
+        '--rulesdir',
+        'lib/rules',
+        '--config',
+        require.resolve('./fixtures/.eslintrcRequiredFirst'),
+        testFilePath,
+        '--fix',
+      ],
+      {
+        encoding: 'utf8',
+      },
+    );
+
+    if (result.status !== 0) {
+      // eslint-disable-next-line no-console
+      console.error(result.stdout);
+      // eslint-disable-next-line no-console
+      console.error(result.stderr);
+      throw new Error(`Process exited with status ${result.status}`);
+    }
+
+    const output = fs.readFileSync(testFilePath, 'utf8');
+
+    assert.strictEqual(output, expected);
+  });
+});

tests/fixtures/.eslintrcRequiredFirst.js

@@ -0,0 +1,14 @@
+module.exports = {
+  root: true,
+  parser: '@typescript-eslint/parser',
+  plugins: ['@typescript-eslint'],
+  rules: {
+    interface: ['error', 'asc', { caseSensitive: true, natural: true, requiredFirst: true }],
+    'string-enum': 'error',
+  },
+  settings: {
+    'import/parsers': {
+      '@typescript-eslint/parser': ['.ts'],
+    },
+  },
+};