refactor(finance)!: rewrite creditCardNumber to work with issuer enum #2720

ST-DDT · 2024-03-05T16:29:48Z

Fixes #181 (our oldest open issue)

Credit Card number parameter #181

(I removed the jsdocs from the implementation signature, as it isn't visible to our users anyway.)

codecov · 2024-03-05T16:33:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (7dc8a18) to head (e73034f).

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #2720      +/-   ##
==========================================
- Coverage   99.96%   99.94%   -0.02%     
==========================================
  Files        2977     2965      -12     
  Lines      215422   215430       +8     
  Branches      950      952       +2     
==========================================
- Hits       215351   215317      -34     
- Misses         71      113      +42

Files	Coverage Δ
src/index.ts	`100.00% <100.00%> (ø)`
.../locales/base/finance/common_credit_card_issuer.ts	`100.00% <100.00%> (ø)`
src/locales/base/finance/index.ts	`100.00% <100.00%> (ø)`
src/locales/base/index.ts	`100.00% <100.00%> (ø)`
src/locales/el/finance/index.ts	`100.00% <ø> (ø)`
...rc/locales/en/finance/common_credit_card_issuer.ts	`100.00% <100.00%> (ø)`
src/locales/en/finance/index.ts	`100.00% <100.00%> (ø)`
...locales/zh_CN/finance/common_credit_card_issuer.ts	`100.00% <100.00%> (ø)`
src/locales/zh_CN/finance/index.ts	`100.00% <100.00%> (ø)`
src/modules/finance/index.ts	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

src/modules/finance/index.ts

ST-DDT · 2024-03-07T17:01:40Z

Team Decision

We will use a literal CreditCardIssuer enum.
To check, whether the credit card patterns contain localized parts.
- If not, remove localized patterns and only use localized data to identify which issuer is preferred in a country

creditCardNumber(options: { issuer?: CreditCardIssuer } = {}) {
  const { issuer = (weighted)arrayElement(defintions.finance.preferred_credit_card_issuer) } = options;

  switch (issuer) {
   case issuer1: ...;
   case issuer2: ...;
}

ST-DDT · 2024-03-09T19:44:05Z

Which variant would you prefer?

// 1A) Using our "options" syntax?
creditCardNumber(options: { issuer?: CreditCardIssuerType } = {})
creditCardNumber({ issuer: CreditCardIssuer.Visa })

or

// 1B) Like person.firstName
creditCardNumber(issuer: CreditCardIssuerType = this.creditCardIssuer())
creditCardNumber(CreditCardIssuer.Visa)

ST-DDT · 2024-03-09T20:14:39Z

Should we allow passing array's of allowed issuers?
And how so?

// 2A) Forbid
... issuer: CreditCardIssuerType ...

or

// 2B) Allow
... issuer: CreditCardIssuerType | CreditCardIssuerType[] ...

or

// 2C) Require
... issuer: CreditCardIssuerType[] ...

test/modules/finance.spec.ts

ST-DDT · 2024-03-11T15:46:45Z

src/modules/finance/index.ts

+        break;
+      }
+
+      default: {


This should throw an error (due to overly exhaustive switch): typescript-eslint/typescript-eslint#8645

Bug: switch-exhaustiveness-check does not consider cases with enum keys typescript-eslint/typescript-eslint#8645

netlify · 2024-03-15T17:09:07Z

✅ Deploy Preview for fakerjs ready!

Name	Link
🔨 Latest commit	`e73034f`
🔍 Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/6648f891d8a5b800081030c2
😎 Deploy Preview	https://deploy-preview-2720.fakerjs.dev
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

matthewmayer · 2024-03-16T04:34:28Z

This should be marked breaking no？ it changes the behavior when a unknown or non lower case issuer is passed.

xDivisionByZerox · 2024-03-17T00:37:17Z

src/modules/finance/index.ts

+    if (typeof issuer !== 'string') {
+      issuer = issuer.issuer ?? this.creditCardIssuer();
+      deprecated({
+        deprecated: 'faker.finance.creditCardNumber({ issuer })',
+        proposed: 'faker.finance.creditCardNumber(issuer)',
+        since: '9.0',
+        until: '10.0',
+      });
    }


Do we really want to deprecate this? "quick options" are a part of fakers signatures in a lot of places. 🤔

xDivisionByZerox · 2024-03-17T00:44:53Z

test/modules/finance.spec.ts

+        .it('with issuer option visa', {
+          issuer: 'visa',
+        } as unknown as CreditCardIssuerType)
+        .it('with issuer option mastercard', {
+          issuer: 'mastercard',
+        } as unknown as CreditCardIssuerType);


Why are these deprecated? 'visa' | 'mastercard' should be included in CreditCardIssuerType. What am I missing?

src/locales/el/finance/credit_card/maestro.ts

src/modules/finance/index.ts

Shinigami92 · 2024-03-17T09:16:08Z

src/modules/finance/index.ts

+    const actualIssuer = issuer.toLowerCase() as CreditCardIssuerType;
+    if (actualIssuer !== issuer) {
+      deprecated({
+        deprecated: 'faker.finance.creditCardNumber(iSsUeR)',


This looks absolutely weird to me 👀

Any idea how to show case insesitive is deprecated?

what about

deprecated: `faker.finance.creditCardNumber('${issuer}')`, proposed: `faker.finance.creditCardNumber('${actualIssuer}')`,

Or maybe:

Suggested change

deprecated: 'faker.finance.creditCardNumber(iSsUeR)',

deprecated: 'faker.finance.creditCardNumber(issuerWithRandomCase)',

Shinigami92 · 2024-03-17T09:17:24Z

src/modules/finance/index.ts

+  AmericanExpress = 'american_express',
+  DinersClub = 'diners_club',


Do we have any conventions yet? Otherwise I would be in favor of using - instead of _ for the string values.

Do we have any conventions yet?

No. I just used what we use/have already.

Since this change is breaking anyway, we might want to standardize these cases. I'm not that much in favor of any case, but would like to have a "rule" for these types.

Shinigami92 · 2024-03-17T09:19:26Z

src/modules/finance/index.ts

+    switch (actualIssuer) {
+      case CreditCardIssuer.AmericanExpress: {


This wont work when the string value of the enum contains a separator like _ or -, but the actuallIssuer is just lowercased in line 580.

Why wouldnt it? The test seem to be working fine.

Ah I see, its because issuer is now CreditCardIssuerType strictly typed instead of a string. But why do we need the toLowerCase then in the first place?

Backwards compatibility.

Is this a discussion about #2725 again? Otherwise the type def is wrong, because arbitrary strings are not possible anymore.

Is this a discussion about #2725 again?

In the sense, that we follow our usual deprecation steps: Yes

That's why the following comment is there:

// TODO @ST-DDT 2024-03-09: Remove the fallback in v10

Wouldn't it be better to do this in two stages? In 9.0 log a runtime warning if an unknown or mixed case issuer is passed. Then in 10.0 switch to the enum.

🤔 Not sure about that.
JS users already have the backwards compatibility code.
TS users would have more time to migrate at the cost of runtime warnings instead of compile time errors.

docs(finance): mention where to look up issuer names

e00d703

ST-DDT added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent m: finance Something is referring to the finance module labels Mar 5, 2024

ST-DDT added this to the vAnytime milestone Mar 5, 2024

ST-DDT requested review from a team March 5, 2024 16:29

ST-DDT self-assigned this Mar 5, 2024

matthewmayer requested changes Mar 5, 2024

View reviewed changes

src/modules/finance/index.ts Outdated Show resolved Hide resolved

docs: list credit card issuers directly

7a26f1e

ST-DDT requested review from matthewmayer and a team March 5, 2024 17:14

Shinigami92 previously approved these changes Mar 5, 2024

View reviewed changes

ST-DDT added the s: needs decision Needs team/maintainer decision label Mar 6, 2024

ST-DDT changed the title ~~docs(finance): mention where to look up issuer names~~ refactor(finance): rewrite creditCardNumber to work an issuer enum Mar 9, 2024

ST-DDT marked this pull request as draft March 9, 2024 19:46

refactor(finance): rewrite credit cards

e18f614

ST-DDT dismissed Shinigami92’s stale review via e18f614 March 9, 2024 20:52

ST-DDT added 3 commits March 9, 2024 22:08

missing files

d56bd62

fix lint issues

13e5649

Merge branch 'next' into docs/finance/credit-card/issuer-source

3557729

ST-DDT changed the title ~~refactor(finance): rewrite creditCardNumber to work an issuer enum~~ refactor(finance): rewrite creditCardNumber to work with issuer enum Mar 10, 2024

ST-DDT added 2 commits March 11, 2024 16:08

Merge branch 'next' into docs/finance/credit-card/issuer-source

82d834a

handle previous code better

43aeb43

ST-DDT commented Mar 11, 2024

View reviewed changes

test/modules/finance.spec.ts Show resolved Hide resolved

Update test/modules/finance.spec.ts

2939cdf

ST-DDT marked this pull request as ready for review March 11, 2024 15:45

ST-DDT commented Mar 11, 2024

View reviewed changes

Merge branch 'next' into docs/finance/credit-card/issuer-source

3974456

ST-DDT changed the title ~~refactor(finance): rewrite creditCardNumber to work with issuer enum~~ refactor(finance)!: rewrite creditCardNumber to work with issuer enum Mar 16, 2024

xDivisionByZerox reviewed Mar 17, 2024

View reviewed changes

src/locales/el/finance/credit_card/maestro.ts Outdated Show resolved Hide resolved

src/modules/finance/index.ts Show resolved Hide resolved

Shinigami92 reviewed Mar 17, 2024

View reviewed changes

ST-DDT removed the s: needs decision Needs team/maintainer decision label Apr 4, 2024

ST-DDT mentioned this pull request Apr 9, 2024

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

Merged

ST-DDT marked this pull request as draft April 11, 2024 06:42

ST-DDT added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release and removed c: docs Improvements or additions to documentation labels Apr 11, 2024

ST-DDT modified the milestones: vAnytime, v9.0 Apr 11, 2024

Merge branch 'next' into docs/finance/credit-card/issuer-source

e73034f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(finance)!: rewrite creditCardNumber to work with issuer enum #2720

refactor(finance)!: rewrite creditCardNumber to work with issuer enum #2720

ST-DDT commented Mar 5, 2024

codecov bot commented Mar 5, 2024 •

edited

ST-DDT commented Mar 7, 2024 •

edited

ST-DDT commented Mar 9, 2024 •

edited

ST-DDT commented Mar 9, 2024

ST-DDT Mar 11, 2024

netlify bot commented Mar 15, 2024 •

edited

matthewmayer commented Mar 16, 2024

xDivisionByZerox Mar 17, 2024 •

edited

xDivisionByZerox Mar 17, 2024

Shinigami92 Mar 17, 2024

ST-DDT Mar 17, 2024

matthewmayer Mar 18, 2024 •

edited

xDivisionByZerox Mar 18, 2024

Shinigami92 Mar 17, 2024

ST-DDT Mar 17, 2024

xDivisionByZerox Mar 18, 2024

Shinigami92 Mar 17, 2024

ST-DDT Mar 17, 2024

Shinigami92 Mar 17, 2024

ST-DDT Mar 17, 2024

Shinigami92 Mar 17, 2024

ST-DDT Mar 17, 2024

matthewmayer Mar 17, 2024

ST-DDT Mar 17, 2024

	deprecated: 'faker.finance.creditCardNumber(iSsUeR)',
	deprecated: 'faker.finance.creditCardNumber(issuerWithRandomCase)',

		AmericanExpress = 'american_express',
		DinersClub = 'diners_club',

		switch (actualIssuer) {
		case CreditCardIssuer.AmericanExpress: {

refactor(finance)!: rewrite creditCardNumber to work with issuer enum #2720

Are you sure you want to change the base?

refactor(finance)!: rewrite creditCardNumber to work with issuer enum #2720

Conversation

ST-DDT commented Mar 5, 2024

codecov bot commented Mar 5, 2024 • edited

Codecov Report

ST-DDT commented Mar 7, 2024 • edited

ST-DDT commented Mar 9, 2024 • edited

ST-DDT commented Mar 9, 2024

Choose a reason for hiding this comment

netlify bot commented Mar 15, 2024 • edited

✅ Deploy Preview for fakerjs ready!

matthewmayer commented Mar 16, 2024

xDivisionByZerox Mar 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewmayer Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2024 •

edited

ST-DDT commented Mar 7, 2024 •

edited

ST-DDT commented Mar 9, 2024 •

edited

netlify bot commented Mar 15, 2024 •

edited

xDivisionByZerox Mar 17, 2024 •

edited

matthewmayer Mar 18, 2024 •

edited