-
-
Notifications
You must be signed in to change notification settings - Fork 922
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): use fake patterns in transactionDescription #3202
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3202 +/- ##
==========================================
- Coverage 99.97% 99.95% -0.02%
==========================================
Files 2806 2806
Lines 217140 217146 +6
Branches 982 975 -7
==========================================
- Hits 217075 217053 -22
- Misses 65 93 +28
|
If you still need the method, please inline/copy the implementation: transactionDescription(): string {
const amount = faker.finance.amount();
const company = faker.company.name();
const transactionType = faker.finance.transactionType();
const account = faker.finance.accountNumber();
const card = faker.finance.creditCardNumber().replaceAll(/(?<=.{4}).(?=.{2})/g, '*');
const currency = faker.finance.currencyCode();
return `${transactionType} transaction at ${company} using card ending with ***${card} for ${currency} ${amount} in account ***${account}`;
} |
A quick GH search says otherwise 🤔
I actually kind of agree with you here. BUT:
Having this method in our codebase doesn't cause a big enought pain point to call for emidiate action in my opinion. |
I tend to agree this doesnt cause any harm and should be left alone. Perhaps we could add a note this is discouraged for new code without formally deprecating it. |
The method only produces English output and isn't usable outside of the English locale.
Note: I'm not against soft-deprecating this method, I just wanted to raise awareness of this fact. If we go for soft-deprecation, how about a text like this:
|
Or alternatively, we convert it to a proper localized method. |
Team Decision
|
The transaction description method is very limited in it's applications.
The method generates multiple values from the finance module and masks and concats them together.
I would argue, that the textual representation of these information is an implementation detail of the UI that will show it.
e.g. it would likely be displayed as
I would consider this a method generating a complex object although it is a string. Which makes the method our of scope for Faker.
Popularity-Data from #3215
faker.finance.transactionDescription
: 20