Skip to content

refactor(locale): remove discontinued credit card issuer "maestro"#2803

Merged
xDivisionByZerox merged 2 commits intonextfrom
refactor/locale/remove-maestro-credit-card-issuer
Apr 15, 2024
Merged

refactor(locale): remove discontinued credit card issuer "maestro"#2803
xDivisionByZerox merged 2 commits intonextfrom
refactor/locale/remove-maestro-credit-card-issuer

Conversation

@xDivisionByZerox
Copy link
Copy Markdown
Member

Description

Removes the discontinued credit card issue "maestro" as described in #2350 (comment).

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: finance Something is referring to the finance module labels Apr 9, 2024
@xDivisionByZerox xDivisionByZerox self-assigned this Apr 9, 2024
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner April 9, 2024 22:33
@xDivisionByZerox xDivisionByZerox requested a review from a team April 9, 2024 22:34
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 9, 2024

Deploy Preview for fakerjs canceled.

Name Link
🔨 Latest commit c73b905
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/661d9210b87704000872b9cd

@xDivisionByZerox
Copy link
Copy Markdown
Member Author

xDivisionByZerox commented Apr 9, 2024

Please tell me if this is considered breaking. Afterwards I can set the correct labels/title/milestone (and add a migration snippet).

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (e53dd37) to head (89311b3).

❗ Current head 89311b3 differs from pull request most recent head c73b905. Consider uploading reports for the commit c73b905 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #2803   +/-   ##
=======================================
  Coverage   99.95%   99.96%           
=======================================
  Files        2966     2969    +3     
  Lines      212579   212530   -49     
  Branches      948      949    +1     
=======================================
- Hits       212490   212445   -45     
+ Misses         89       85    -4     
Files Coverage Δ
src/locales/el/finance/credit_card/index.ts 100.00% <ø> (ø)
src/locales/en/finance/credit_card/index.ts 100.00% <ø> (ø)

... and 29 files with indirect coverage changes

@ST-DDT
Copy link
Copy Markdown
Member

ST-DDT commented Apr 9, 2024

Please tell me if this is considered breaking. Afterwards I can set the correct labels/title/milestone (and add a migration snippet).

I would consider it breaking because a "well known value" will stop working even if the type does not cover it.


See also #2720

@ST-DDT ST-DDT requested a review from a team April 9, 2024 22:50
@ST-DDT
Copy link
Copy Markdown
Member

ST-DDT commented Apr 9, 2024

Are the other credit card issuers valid?

@xDivisionByZerox
Copy link
Copy Markdown
Member Author

xDivisionByZerox commented Apr 9, 2024

Are the other credit card issuers valid?

This is not supposed to be a fix for #2350, as removing a discontinued issue is different from fixing invalid patterns, based on an issue, IMO.

What I want to say is: I did not check the other pattern as I would do so in a different PR (which is not breaking as it is a fix)

@matthewmayer
Copy link
Copy Markdown
Contributor

Please tell me if this is considered breaking. Afterwards I can set the correct labels/title/milestone (and add a migration snippet).

I would consider it breaking because a "well known value" will stop working even if the type does not cover it.


See also #2720

I think it's worth a quick mention in the migration guide to note that;

  • creditCardNumber with no options will never return maestro card numbers any more
  • creditCardNumber({issuer:"maestro"}) will fall back to returning a number from a random issuer.

@ST-DDT
Copy link
Copy Markdown
Member

ST-DDT commented Apr 10, 2024

  • creditCardNumber({issuer:"maestro"}) will fall back to returning a number from a random issuer.

And any other method that would have an option removed/renamed would work as well.
Only in this case we dont even get a ts error.
IMO MaestroCreditCard != random CreditCard

Just for clarification: I dont require a migration guide, if #2720 is merged after this before any release.

@matthewmayer
Copy link
Copy Markdown
Contributor

for reference we didn't consider #2344 breaking

@xDivisionByZerox xDivisionByZerox enabled auto-merge (squash) April 15, 2024 20:46
@xDivisionByZerox xDivisionByZerox changed the title refactor(locale): remove discontinued credit card issue "maestro" refactor(locale): remove discontinued credit card issuer "maestro" Apr 15, 2024
@xDivisionByZerox xDivisionByZerox enabled auto-merge (squash) April 15, 2024 20:46
@xDivisionByZerox xDivisionByZerox merged commit cd5577c into next Apr 15, 2024
@ST-DDT ST-DDT deleted the refactor/locale/remove-maestro-credit-card-issuer branch April 17, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: locale Permutes locale definitions m: finance Something is referring to the finance module p: 1-normal Nothing urgent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants