Skip to content

Commit

Permalink
feat(eslint-plugin): [member-ordering] add a required option for requ…
Browse files Browse the repository at this point in the history
…ired vs. optional member ordering (#5965)

* fix(eslint-plugin): [member-ordering] add requiredFirst as an option which ensures that all required members appear before all optional members.

* fix(eslint-plugin): [member-ordering] adding types so build passes.

* fix(eslint-plugin): [member-ordering] fixing types so build passes.

* fix(eslint-plugin): [member-ordering] refactoring getIndexOfLastRequiredMember to be slightly faster and adding jsdoc comments for it and isMemberOptional.

* fix(eslint-plugin): [member-ordering] additional test cases and handling for them.

* fix(eslint-plugin): [member-ordering] linting fix.

* fix(eslint-plugin): [member-ordering] change requiredFirst to required which takes first or last as a value and adding functionality to check order based on both of these along with additional tests.

* fix(eslint-plugin): [member-ordering] refactoring according to PR comments.

* fix(eslint-plugin): [member-ordering] refactoring for PR and adding another test case.

* fix(eslint-plugin): [member-ordering] refactoring for PR.

* fix(eslint-plugin): [member-ordering] adding test cases for coverage and removing unused code.

* fix(eslint-plugin): [member-ordering] increasing coverage to pass check.

* feat(eslint-plugin): [member-ordering] adding more tests to increase coverage for isMemberOptional function.

* Updated name to optionalityOrder

Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>
  • Loading branch information
asdf93074 and JoshuaKGoldberg committed Nov 28, 2022
1 parent becd1f8 commit 2abadc6
Show file tree
Hide file tree
Showing 5 changed files with 718 additions and 17 deletions.
94 changes: 93 additions & 1 deletion packages/eslint-plugin/docs/rules/member-ordering.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type OrderConfig = MemberType[] | SortedOrderConfig | 'never';

interface SortedOrderConfig {
memberTypes?: MemberType[] | 'never';
optionalityOrder?: 'optional-first' | 'required-first';
order:
| 'alphabetically'
| 'alphabetically-case-insensitive'
Expand All @@ -44,9 +45,10 @@ You can configure `OrderConfig` options for:
- **`interfaces`**?: override ordering specifically for interfaces
- **`typeLiterals`**?: override ordering specifically for type literals

The `OrderConfig` settings for each kind of construct may configure sorting on one or both two levels:
The `OrderConfig` settings for each kind of construct may configure sorting on up to three levels:

- **`memberTypes`**: organizing on member type groups such as methods vs. properties
- **`optionalityOrder`**: whether to put all optional members first or all required members first
- **`order`**: organizing based on member names, such as alphabetically

### Groups
Expand Down Expand Up @@ -911,6 +913,96 @@ interface Foo {
}
```

#### Sorting Optional Members First or Last

The `optionalityOrder` option may be enabled to place all optional members in a group at the beginning or end of that group.

This config places all optional members before all required members:

```jsonc
// .eslintrc.json
{
"rules": {
"@typescript-eslint/member-ordering": [
"error",
{
"default": {
"optionalityOrder": "optional-first",
"order": "alphabetically"
}
}
]
}
}
```

<!--tabs-->

##### ❌ Incorrect

```ts
interface Foo {
a: boolean;
b?: number;
c: string;
}
```

##### ✅ Correct

```ts
interface Foo {
b?: number;
a: boolean;
c: string;
}
```

<!--/tabs-->

This config places all required members before all optional members:

```jsonc
// .eslintrc.json
{
"rules": {
"@typescript-eslint/member-ordering": [
"error",
{
"default": {
"optionalityOrder": "required-first",
"order": "alphabetically"
}
}
]
}
}
```

<!--tabs-->

##### ❌ Incorrect

```ts
interface Foo {
a: boolean;
b?: number;
c: string;
}
```

##### ✅ Correct

```ts
interface Foo {
a: boolean;
c: string;
b?: number;
}
```

<!--/tabs-->

## All Supported Options

### Member Types (Granular Form)
Expand Down
145 changes: 129 additions & 16 deletions packages/eslint-plugin/src/rules/member-ordering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import naturalCompare from 'natural-compare-lite';

import * as util from '../util';

export type MessageIds = 'incorrectGroupOrder' | 'incorrectOrder';
export type MessageIds =
| 'incorrectGroupOrder'
| 'incorrectOrder'
| 'incorrectRequiredMembersOrder';

type MemberKind =
| 'call-signature'
Expand Down Expand Up @@ -47,12 +50,15 @@ type Order = AlphabeticalOrder | 'as-written';

interface SortedOrderConfig {
memberTypes?: MemberType[] | 'never';
optionalityOrder?: OptionalityOrder;
order: Order;
}

type OrderConfig = MemberType[] | SortedOrderConfig | 'never';
type Member = TSESTree.ClassElement | TSESTree.TypeElement;

type OptionalityOrder = 'optional-first' | 'required-first';

export type Options = [
{
default?: OrderConfig;
Expand Down Expand Up @@ -101,6 +107,10 @@ const objectConfig = (memberTypes: MemberType[]): JSONSchema.JSONSchema4 => ({
'natural-case-insensitive',
],
},
optionalityOrder: {
type: 'string',
enum: ['optional-first', 'required-first'],
},
},
additionalProperties: false,
});
Expand Down Expand Up @@ -405,6 +415,26 @@ function getMemberName(
}
}

/**
* Returns true if the member is optional based on the member type.
*
* @param node the node to be evaluated.
*
* @returns Whether the member is optional, or false if it cannot be optional at all.
*/
function isMemberOptional(node: Member): boolean {
switch (node.type) {
case AST_NODE_TYPES.TSPropertySignature:
case AST_NODE_TYPES.TSMethodSignature:
case AST_NODE_TYPES.TSAbstractPropertyDefinition:
case AST_NODE_TYPES.PropertyDefinition:
case AST_NODE_TYPES.TSAbstractMethodDefinition:
case AST_NODE_TYPES.MethodDefinition:
return !!node.optional;
}
return false;
}

/**
* Gets the calculated rank using the provided method definition.
* The algorithm is as follows:
Expand Down Expand Up @@ -561,6 +591,7 @@ export default util.createRule<Options, MessageIds>({
'Member {{member}} should be declared before member {{beforeMember}}.',
incorrectGroupOrder:
'Member {{name}} should be declared before all {{rank}} definitions.',
incorrectRequiredMembersOrder: `Member {{member}} should be declared after all {{optionalOrRequired}} members.`,
},
schema: [
{
Expand Down Expand Up @@ -725,6 +756,59 @@ export default util.createRule<Options, MessageIds>({
}
}

/**
* Checks if the order of optional and required members is correct based
* on the given 'required' parameter.
*
* @param members Members to be validated.
* @param optionalityOrder Where to place optional members, if not intermixed.
*
* @return True if all required and optional members are correctly sorted.
*/
function checkRequiredOrder(
members: Member[],
optionalityOrder: OptionalityOrder | undefined,
): boolean {
const switchIndex = members.findIndex(
(member, i) =>
i && isMemberOptional(member) !== isMemberOptional(members[i - 1]),
);

const report = (member: Member): void =>
context.report({
messageId: 'incorrectRequiredMembersOrder',
loc: member.loc,
data: {
member: getMemberName(member, context.getSourceCode()),
optionalOrRequired:
optionalityOrder === 'optional-first' ? 'required' : 'optional',
},
});

// if the optionality of the first item is correct (based on optionalityOrder)
// then the first 0 inclusive to switchIndex exclusive members all
// have the correct optionality
if (
isMemberOptional(members[0]) !==
(optionalityOrder === 'required-first')
) {
report(members[0]);
return false;
}

for (let i = switchIndex + 1; i < members.length; i++) {
if (
isMemberOptional(members[i]) !==
isMemberOptional(members[switchIndex])
) {
report(members[switchIndex]);
return false;
}
}

return true;
}

/**
* Validates if all members are correctly sorted.
*
Expand All @@ -743,33 +827,62 @@ export default util.createRule<Options, MessageIds>({

// Standardize config
let order: Order | undefined;
let memberTypes;
let memberTypes: string | MemberType[] | undefined;
let optionalityOrder: OptionalityOrder | undefined;

// returns true if everything is good and false if an error was reported
const checkOrder = (memberSet: Member[]): boolean => {
const hasAlphaSort = !!(order && order !== 'as-written');

// Check order
if (Array.isArray(memberTypes)) {
const grouped = checkGroupSort(
memberSet,
memberTypes,
supportsModifiers,
);

if (grouped === null) {
return false;
}

if (hasAlphaSort) {
return !grouped.some(
groupMember =>
!checkAlphaSort(groupMember, order as AlphabeticalOrder),
);
}
} else if (hasAlphaSort) {
return checkAlphaSort(memberSet, order as AlphabeticalOrder);
}

return true;
};

if (Array.isArray(orderConfig)) {
memberTypes = orderConfig;
} else {
order = orderConfig.order;
memberTypes = orderConfig.memberTypes;
optionalityOrder = orderConfig.optionalityOrder;
}

const hasAlphaSort = !!(order && order !== 'as-written');
if (!optionalityOrder) {
checkOrder(members);
return;
}

// Check order
if (Array.isArray(memberTypes)) {
const grouped = checkGroupSort(members, memberTypes, supportsModifiers);
const switchIndex = members.findIndex(
(member, i) =>
i && isMemberOptional(member) !== isMemberOptional(members[i - 1]),
);

if (grouped === null) {
if (switchIndex !== -1) {
if (!checkRequiredOrder(members, optionalityOrder)) {
return;
}

if (hasAlphaSort) {
grouped.some(
groupMember =>
!checkAlphaSort(groupMember, order as AlphabeticalOrder),
);
}
} else if (hasAlphaSort) {
checkAlphaSort(members, order as AlphabeticalOrder);
checkOrder(members.slice(0, switchIndex));
checkOrder(members.slice(switchIndex));
}
}

Expand Down
24 changes: 24 additions & 0 deletions packages/eslint-plugin/src/util/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,29 @@ function formatWordList(words: string[]): string {
return [words.slice(0, -1).join(', '), words.slice(-1)[0]].join(' and ');
}

/**
* Iterates the array in reverse and returns the index of the first element it
* finds which passes the predicate function.
*
* @returns Returns the index of the element if it finds it or -1 otherwise.
*/
function findLastIndex<T>(
members: T[],
predicate: (member: T) => boolean | undefined | null,
): number {
let idx = members.length - 1;

while (idx >= 0) {
const valid = predicate(members[idx]);
if (valid) {
return idx;
}
idx--;
}

return -1;
}

export {
arrayGroupByToMap,
arraysAreEqual,
Expand All @@ -194,4 +217,5 @@ export {
MemberNameType,
RequireKeys,
upperCaseFirst,
findLastIndex,
};

0 comments on commit 2abadc6

Please sign in to comment.