Skip to content
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

infra(unicorn): no-useless-switch-case #2508

Merged
merged 14 commits into from
Feb 20, 2024

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 27, 2023

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: infra Changes to our infrastructure or project setup labels Oct 27, 2023
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 27, 2023
@ST-DDT ST-DDT requested review from a team October 27, 2023 20:30
@ST-DDT ST-DDT self-assigned this Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (25f2a03) 99.55% compared to head (5be7f09) 99.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2508      +/-   ##
==========================================
- Coverage   99.55%   99.55%   -0.01%     
==========================================
  Files        2817     2817              
  Lines      251208   251188      -20     
  Branches      725      718       -7     
==========================================
- Hits       250101   250075      -26     
- Misses       1078     1113      +35     
+ Partials       29        0      -29     
Files Coverage Δ
src/modules/color/index.ts 99.79% <ø> (-0.01%) ⬇️
src/modules/internet/index.ts 100.00% <100.00%> (ø)
src/modules/location/index.ts 99.05% <ø> (-0.16%) ⬇️
src/modules/string/index.ts 100.00% <ø> (ø)

... and 30 files with indirect coverage changes

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Oct 30, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 31, 2023

Team Decision

  • We want to enable https://typescript-eslint.io/rules/switch-exhaustiveness-check/
  • After that is merged, we no longer need the default and can remove it.
    • Consideration, if a non typescript user inputs a bad value, currently it would go into the default case. We could throw an error but we don't do that for other bad values either, so non typescript users don't get special support for enums either.
  • @ST-DDT: Will create an issue for switch-exhaustiveness-checks to require default cases for non union type switch cases

EDIT:

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Oct 31, 2023
@ST-DDT ST-DDT marked this pull request as draft October 31, 2023 18:29
@ST-DDT ST-DDT added s: on hold Blocked by something or frozen to avoid conflicts s: accepted Accepted feature / Confirmed bug labels Oct 31, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 1, 2023

@ST-DDT ST-DDT removed the s: on hold Blocked by something or frozen to avoid conflicts label Nov 9, 2023
@ST-DDT ST-DDT marked this pull request as ready for review November 9, 2023 22:41
@ST-DDT ST-DDT requested review from xDivisionByZerox and a team November 9, 2023 22:41
Copy link
Contributor

@matthewmayer matthewmayer left a comment

Choose a reason for hiding this comment

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

Support merging in v9 so we can document the breaking change for non-TS users.

@ST-DDT ST-DDT removed this from the vAnytime milestone Nov 10, 2023
@ST-DDT ST-DDT added this to the v9.0 milestone Nov 10, 2023
@ST-DDT ST-DDT added the do NOT merge yet Do not merge this PR into the target branch yet label Nov 10, 2023
@xDivisionByZerox xDivisionByZerox added the breaking change Cannot be merged when next version is not a major release label Dec 4, 2023
@matthewmayer
Copy link
Contributor

matthewmayer commented Jan 6, 2024

Per #2521 suggest adding the following to docs/guide/upgrading_v9/2508.md:

Some methods now return undefined in Javascript when unknown enumeration values are passed

Some methods would previously fallback to a default value for an option when an unknown value was passed for a enum parameter. Now, these methods will return undefined instead. This only affects usage in Javascript, as in Typescript this usage would already throw a compile-time error.

For example:

faker.color.rgb({format: "unexpectedvalue"}) 
// in Faker v8, is [ 110, 82, 190 ] like {format: "decimal"}
// in Faker v9, is undefined

This affects:

  • The format property of faker.color.rgb() must be one of 'binary' | 'css' | 'decimal' | 'hex' if provided
  • The format property of faker.color.cmyk(), faker.color.hsl(), faker.color.hwb(), faker.color.lab(), faker.color.lch() must be one of 'binary' | 'css' | 'decimal' if provided
  • The variant property of faker.location.countryCode() must be one of alpha-2, alpha-3, numeric if provided
  • The casing property of faker.string.alpha() and faker.string.alphanumeric() must be one of 'upper' | 'lower' | 'mixed' if provided

@ST-DDT ST-DDT removed the do NOT merge yet Do not merge this PR into the target branch yet label Feb 8, 2024
@ST-DDT ST-DDT requested a review from a team February 8, 2024 19:01
@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 8, 2024

Per #2521 suggest adding the following to docs/guide/upgrading_v9/2508.md:

Added. Thanks for the suggestion.

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 8, 2024

Ready for review.

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

theoretically approved, but please let someone like e.g. @xDivisionByZerox evaluate my suggestion 🙂

src/modules/internet/index.ts Outdated Show resolved Hide resolved
src/modules/internet/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested review from matthewmayer and a team February 19, 2024 10:42
@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 19, 2024

Switched to strategy pattern. Ready to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants