Movatterモバイル変換

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🆒

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx OutdatedShow resolvedHide resolved

packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx OutdatedShow resolvedHide resolved

packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts OutdatedShow resolvedHide resolved

packages/eslint-plugin/tests/rules/switch-exhaustiveness-check.test.ts OutdatedShow resolvedHide resolved

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Sep 30, 2024

fix: apply code reivew

fb1406a

developer-bandi requested a review fromJoshuaKGoldberg

October 3, 2024 12:17

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Oct 3, 2024

kirkwaiblinger reviewed

Oct 9, 2024

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The code logic looks great to me, but some cleanup is needed for the option naming and docs.

allowDefaultCaseMatchUnionMember is ungrammatical, andrequireDefaultCaseForUnions is not accurate, to my understanding.

MayberequireExplicitCasesForUnion orconsiderDefaultExhaustiveForUnion depending on which direction the true/false goes?

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx Outdated

		@@ -53,6 +53,29 @@ switch (value) {

		Since `value` is a non-union type it requires the switch case to have a default clause only with `requireDefaultForNonUnion` enabled.

		### `allowDefaultCaseMatchUnionMember`

Copy link

Member

kirkwaiblingerOct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

option name in docs differs from code

packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Outdated

		@@ -155,10 +173,13 @@ export default createRule<Options, MessageIds>({
		const { missingLiteralBranchTypes, symbolName, defaultCase } =
		switchMetadata;

		// We only trigger the rule if a `default` case does not exist, since that
		// would disqualify the switch statement from having cases that exactly
		// We only trigger the rule if a `default` case does not exist when requireDefaultCaseForUnions option

Copy link

Member

kirkwaiblingerOct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(clarity nit)

Maybe just have a standalone condition like

// Unless requireDefaultCaseForUnions is enabled, the presence of a default case// always makes the switch exhaustive.if(!requireDefaultCaseForUnions&&defaultCase!=null){return;}

packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Outdated

		@@ -243,6 +264,15 @@ export default createRule<Options, MessageIds>({
		.join('\n');

		if (lastCase) {
		const isLastCaseDefaultCase = lastCase.test == null;
		if (isLastCaseDefaultCase) {

Copy link

Member

kirkwaiblingerOct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(optional)

food for thought - what if the default case isn't the last case?

This change makes it so that we avoid suggesting to fix

declareconstliteral:'a'|'b';switch(literal){case'b':default:}

declareconstliteral:'a'|'b';switch(literal){case'b':default:case"a":{thrownewError('Not implemented yet: "a" case')}}

and instead fix it to

declareconstliteral:'a'|'b';switch(literal){case'b':case"a":{thrownewError('Not implemented yet: "a" case')}default:}

. (also note that this has its own issue of no longer havingcase 'b' be handled the same asdefault, instead having it throw as not implemented yet).

But it will happily fix

declareconstliteral:'a'|'b';switch(literal){default:case'b':}

declareconstliteral:'a'|'b';switch(literal){default:case'b':case"a":{thrownewError('Not implemented yet: "a" case')}}

which still has the added case as part of the default 🤔, even though the first examples implies this code is trying to avoid it. Maybe to be more consistent we could say "if default case is present, add it above it, else add it at the end"?

I don't think that this is super important since the user will have to do manual cleanup regardless, but just throwing this out there in case you want to tweak some edge cases. I'm ok with either way though.

packages/eslint-plugin/src/rules/switch-exhaustiveness-check.ts Outdated

		@@ -65,6 +72,10 @@ export default createRule<Options, MessageIds>({
		description: `If 'true', require a 'default' clause for switches on non-union types.`,
		type: 'boolean',
		},
		requireDefaultCaseForUnions: {
		description: `If 'true', the 'default' clause is used to determine whether the switch statement is Exhaustive for union type`,

Copy link

Member

kirkwaiblingerOct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

	description:`If 'true', the 'default' clause is used to determine whether the switch statement isExhaustive for union type`,
	description:`If 'true', the 'default' clause is used to determine whether the switch statement isexhaustive for union type`,

kirkwaiblinger reviewed

Oct 9, 2024

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdxShow resolvedHide resolved

Copy link

Member

kirkwaiblinger commentedOct 9, 2024•
edited
Loading

(Further thoughts on naming and options) -

My understanding is that there are two independent things going on with union types and defaults.

Whenall union constituentsdo have their own explicit case, whether adefault case is prohibited|ignored|required. This is currently controlled by the existingallowDefaultCaseForExhaustiveSwitch option, and the only possibilities currently supported by this rule are prohibited|ignored (If you want the "required" behavior you can use the separate ruledefault-case).
When some union constituentsdo not have their own explicit case, whether adefault case will silence the rule. This is the new option being created.

Perhaps thinking about it in this framework can help clarify the docs and the naming.

When I readrequireDefaultCaseForUnions, that describes the behavior in 1., not in 2.

kirkwaiblinger added awaiting response

Issues waiting for a reply from the OP or another party

enhancementNew feature or request labels

Oct 9, 2024

Copy link

ContributorAuthor

developer-bandi commentedOct 10, 2024•
edited
Loading

As for the option name, theconsiderDefaultExhaustiveForUnion you suggested looks good.

It seems that including bothDefault andExhaustive keywords is more appropriate in the context you explained.

Once the option name is decided, related modifications will be reflected.

fix: reflect code review

bef9823

developer-bandi requested a review fromkirkwaiblinger

October 10, 2024 16:37

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Oct 10, 2024

kirkwaiblinger reviewed

Oct 14, 2024

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx OutdatedShow resolvedHide resolved

Copy link

Member

kirkwaiblinger commentedOct 14, 2024

looks pretty close. Let's just change the option name toconsiderDefaultExhaustiveForUnions and if anyone else has a conflicting opinion they can object before it gets merged 👍

developer-bandi added2 commits

October 15, 2024 22:49

docs:

c76867c

Remove unnecessary phrases

docs: change option name

e0c892e

kirkwaiblinger reviewed

Oct 15, 2024

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx Outdated


		Defaults to false. Whether a `default` clause will be considered as making a switch statement over a union type exhaustive.

		By default, if a `switch` statement over a union type includes a `default` case, then it's considered exhaustive.

Copy link

Member

kirkwaiblingerOct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this is the opposite way (since the option defaults to false). By default, every branch will be required to be explicitly handled, and one may opt out by enablingconsiderDefaultExhaustiveForUnions

Copy link

ContributorAuthor

developer-bandiOct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

thank you I think I missed changing it at the same time when changing the default value.

kirkwaiblinger requested changes

Oct 15, 2024

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just the nit inhttps://github.com/typescript-eslint/typescript-eslint/pull/9954/files#r1801500418

docs: considerDefaultExhaustiveForUnions option description edit

55f761f

kirkwaiblinger requested changes

Oct 16, 2024

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx OutdatedShow resolvedHide resolved

fix:lint error

d594720

developer-bandi requested a review fromkirkwaiblinger

October 19, 2024 15:07

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Oct 19, 2024

kirkwaiblinger added3 commits

October 27, 2024 00:50

fix heading issue

718c6bb

refactor to tabs

1203a7c

Merge branch 'main' into feature/switch-exhaustiveness-check

b03b724

kirkwaiblinger previously approved these changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good stuff! I'm super excited to get this change released! This has been a pain point for me as a user of the rule, and I'm glad to see it addressed 🙂

FYI - I've made a few edits to your branch primarily in order to get the website to build (we added in the{/* insert option description */} thing recently, which prevented the CI from building).

kirkwaiblinger added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

kirkwaiblinger dismissedJoshuaKGoldberg’sstale review

October 27, 2024 07:13

stale

kirkwaiblinger requested review fromJoshuaKGoldberg and removed request forJoshuaKGoldberg

October 27, 2024 07:14

github-actionsbot removed the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label

developer-bandi mentioned this pull request

feat(eslint-plugin): [switch-exhaustiveness-check] add support for "no default" comment#10218

Merged

3 tasks

JoshuaKGoldberg previously approved these changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM :)

packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx OutdatedShow resolvedHide resolved

Update packages/eslint-plugin/docs/rules/switch-exhaustiveness-check.mdx

3e1a24e

JoshuaKGoldberg dismissed stale reviews fromkirkwaiblinger and themself via3e1a24e

October 27, 2024 15:00

fix text snapshots

76c8716

JoshuaKGoldberg merged commit3c8978d intotypescript-eslint:main

62 checks passed

kirkwaiblinger changed the title~~feat(eslint-plugin): [switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option~~feat(eslint-plugin): [switch-exhaustiveness-check] add considerDefaultExhaustiveForUnion option

kirkwaiblinger changed the title~~feat(eslint-plugin): [switch-exhaustiveness-check] add considerDefaultExhaustiveForUnion option~~feat(eslint-plugin): [switch-exhaustiveness-check] add considerDefaultExhaustiveForUnions option

renovatebot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request

fix(deps): update typescript-eslint monorepo to v8.12.0

b74caac

##### [v8.12.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8120-2024-10-28)##### 🚀 Features-   **eslint-plugin:** \[no-base-to-string] handle String() ([#10005](typescript-eslint/typescript-eslint#10005))-   **eslint-plugin:** \[switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option ([#9954](typescript-eslint/typescript-eslint#9954))-   **eslint-plugin:** \[consistent-indexed-object-style] report mapped types ([#10160](typescript-eslint/typescript-eslint#10160))-   **eslint-plugin:** \[prefer-nullish-coalescing] add support for assignment expressions ([#10152](typescript-eslint/typescript-eslint#10152))##### ❤️  Thank You-   Abraham Guo-   Kim Sang Du [@developer-bandi](https://github.com/developer-bandi)-   Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger)-   YeonJuan [@yeonjuan](https://github.com/yeonjuan)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

kirkwaiblinger mentioned this pull request