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
refactor(finance)!: rewrite creditCardNumber to work with issuer enum #2720
base: next
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
Team Decision
creditCardNumber(options: { issuer?: CreditCardIssuer } = {}) {
const { issuer = (weighted)arrayElement(defintions.finance.preferred_credit_card_issuer) } = options;
switch (issuer) {
case issuer1: ...;
case issuer2: ...;
} |
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) |
Should we allow passing array's of allowed issuers? // 2A) Forbid
... issuer: CreditCardIssuerType ... or // 2B) Allow
... issuer: CreditCardIssuerType | CreditCardIssuerType[] ... or // 2C) Require
... issuer: CreditCardIssuerType[] ... |
break; | ||
} | ||
|
||
default: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should throw an error (due to overly exhaustive switch): typescript-eslint/typescript-eslint#8645
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This should be marked breaking no? it changes the behavior when a unknown or non lower case issuer is passed. |
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', | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to deprecate this? "quick options" are a part of fakers signatures in a lot of places. 🤔
.it('with issuer option visa', { | ||
issuer: 'visa', | ||
} as unknown as CreditCardIssuerType) | ||
.it('with issuer option mastercard', { | ||
issuer: 'mastercard', | ||
} as unknown as CreditCardIssuerType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these deprecated? 'visa' | 'mastercard'
should be included in CreditCardIssuerType
. What am I missing?
const actualIssuer = issuer.toLowerCase() as CreditCardIssuerType; | ||
if (actualIssuer !== issuer) { | ||
deprecated({ | ||
deprecated: 'faker.finance.creditCardNumber(iSsUeR)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks absolutely weird to me 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea how to show case insesitive is deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
deprecated: `faker.finance.creditCardNumber('${issuer}')`,
proposed: `faker.finance.creditCardNumber('${actualIssuer}')`,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe:
deprecated: 'faker.finance.creditCardNumber(iSsUeR)', | |
deprecated: 'faker.finance.creditCardNumber(issuerWithRandomCase)', |
AmericanExpress = 'american_express', | ||
DinersClub = 'diners_club', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any conventions yet? Otherwise I would be in favor of using -
instead of _
for the string values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any conventions yet?
No. I just used what we use/have already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
switch (actualIssuer) { | ||
case CreditCardIssuer.AmericanExpress: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wont work when the string value of the enum contains a separator like _
or -
, but the actuallIssuer
is just lowercased in line 580.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldnt it? The test seem to be working fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a discussion about #2725 again? Otherwise the type def is wrong, because arbitrary strings are not possible anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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.
Fixes #181 (our oldest open issue)
(I removed the jsdocs from the implementation signature, as it isn't visible to our users anyway.)