Skip to content

add requiredFirst option #13

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

Merged
merged 8 commits into from
May 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 59 additions & 2 deletions docs/rules/interface.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -88,7 +102,7 @@ interface U {
"typescript-sort-keys/interface": [
"error",
"asc",
{ "caseSensitive": true, "natural": false }
{ "caseSensitive": true, "natural": false, "requiredFirst": false }
]
}
```
Expand All @@ -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:

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
32 changes: 27 additions & 5 deletions lib/plugin.js
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down Expand Up @@ -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('');
Expand All @@ -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 => [
Expand All @@ -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);
Expand All @@ -179,6 +200,7 @@ module.exports = function createReporter(context, createReportObject) {
order,
insensitive: insensitive ? 'insensitive ' : '',
natural: natural ? 'natural ' : '',
requiredFirst: requiredFirst ? 'required first ' : '',
...data,
},
fix: fixer => {
Expand Down
5 changes: 4 additions & 1 deletion lib/rules/interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ module.exports = {
natural: {
type: 'boolean',
},
requiredFirst: {
type: 'boolean',
},
},
additionalProperties: false,
},
Expand All @@ -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 {
Expand Down
51 changes: 38 additions & 13 deletions lib/utils/ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
},

/**
Expand All @@ -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;
},
};
49 changes: 49 additions & 0 deletions tests/autofix.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
14 changes: 14 additions & 0 deletions tests/fixtures/.eslintrcRequiredFirst.js
Original file line number Diff line number Diff line change
@@ -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'],
},
},
};
Loading