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

refactor(finance): standardize arguments #1821

Merged
merged 8 commits into from
Feb 15, 2023

Conversation

xDivisionByZerox
Copy link
Member

Part of #1349.

Standardize all function signatures in the FinanceModule to support an options object (if they have parameters).

@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: finance Something is referring to the finance module labels Feb 11, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team February 11, 2023 16:11
@xDivisionByZerox xDivisionByZerox self-assigned this Feb 11, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner February 11, 2023 16:11
@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Feb 11, 2023

I have the save issue which I had here. This time I also did pnpm run build beforehand. Still, no files changed locally.

Deleting test/scripts/apidoc/temp/ as @Shinigami92 described didn't work for me as well. Neither did deleting the snapshot files itself.

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Feb 11, 2023

Ok, I took a closer look into the actual errors in the pipeline. Due to my locale machine being setup with different localization than the CI one and faker.finance.amount being able to return a localized value the snapshots will always be different.

A quick fix would be to only call parameters for the snapshots which take the "non-localized" code path. But that doesn't solve the real issue behind this.

@matthewmayer
Copy link
Contributor

I feel it should call toLocaleString(faker.locale) so it's not dependent on your current environments locale

@ST-DDT
Copy link
Member

ST-DDT commented Feb 12, 2023

The faker.locale as defined right now is unsuitable for that as it contains more of an arbritary name instead of a supported locale code.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 12, 2023

Due to my locale machine being setup with different localization than the CI one and faker.finance.amount being able to return a localized value the snapshots will always be different.

What changed so that you only get the error now and not before?
Did you add a test that wasnt there before?

@matthewmayer
Copy link
Contributor

The faker.locale as defined right now is unsuitable for that as it contains more of an arbitrary name instead of a supported locale code.

If you replace - with _ you usually have a valid locale string (en, es_MX etc). As a fallback, you could catch a Uncaught RangeError: Incorrect locale information provided error and fallback to en.

@matthewmayer
Copy link
Contributor

The faker.locale as defined right now is unsuitable for that as it contains more of an arbitrary name instead of a supported locale code.

If you replace - with _ you usually have a valid locale string (en, es_MX etc). As a fallback, you could catch a Uncaught RangeError: Incorrect locale information provided error and fallback to en.

It seems all the locales work without errors if you do this replacement:

Object.keys(faker.locales).forEach(fakerLocale => {
    try {
        const [validLocale] = Intl.getCanonicalLocales(fakerLocale.replace(/_/g, '-'));
        console.log(fakerLocale, validLocale, ((1234567.89).toLocaleString(validLocale)))
    } catch (e) {
        console.error("ERROR ON " + fakerLocale +" " +e)
    }
})
af_ZA af-ZA 1 234 567,89
ar ar ١٬٢٣٤٬٥٦٧٫٨٩
az az 1.234.567,89
cz cz 1,234,567.89
de de 1.234.567,89
de_AT de-AT 1 234 567,89
de_CH de-CH 1’234’567.89
dv dv 1,234,567.89
el el 1.234.567,89
en en 1,234,567.89
en_AU en-AU 1,234,567.89
en_AU_ocker en-AU-ocker 1,234,567.89
en_BORK en-Bork 1,234,567.89
en_CA en-CA 1,234,567.89
en_GB en-GB 1,234,567.89
en_GH en-GH 1,234,567.89
en_IE en-IE 1,234,567.89
en_IN en-IN 12,34,567.89
en_NG en-NG 1,234,567.89
en_US en-US 1,234,567.89
en_ZA en-ZA 1 234 567,89
es es 1.234.567,89
es_MX es-MX 1,234,567.89
fa fa ۱٬۲۳۴٬۵۶۷٫۸۹
fi fi 1 234 567,89
fr fr 1 234 567,89
fr_BE fr-BE 1 234 567,89
fr_CA fr-CA 1 234 567,89
fr_CH fr-CH 1 234 567,89
fr_LU fr-LU 1.234.567,89
ge ge 1,234,567.89
he he 1,234,567.89
hr hr 1.234.567,89
hu hu 1 234 567,89
hy hy 1 234 567,89
id_ID id-ID 1.234.567,89
it it 1.234.567,89
ja ja 1,234,567.89
ko ko 1,234,567.89
lv lv 1 234 567,89
mk mk 1.234.567,89
nb_NO nb-NO 1 234 567,89
ne ne १२,३४,५६७.८९
nl nl 1.234.567,89
nl_BE nl-BE 1.234.567,89
pl pl 1 234 567,89
pt_BR pt-BR 1.234.567,89
pt_PT pt-PT 1 234 567,89
ro ro 1.234.567,89
ru ru 1 234 567,89
sk sk 1 234 567,89
sv sv 1 234 567,89
tr tr 1.234.567,89
uk uk 1 234 567,89
ur ur 1,234,567.89
vi vi 1.234.567,89
zh_CN zh-CN 1,234,567.89
zh_TW zh-TW 1,234,567.89
zu_ZA zu-ZA 1,234,567.89

@xDivisionByZerox
Copy link
Member Author

What changed so that you only get the error now and not before?
Did you add a test that wasnt there before?

Yes. In amount() the autoFormatted parameter was not tested before. So I added a case for that.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 13, 2023

What changed so that you only get the error now and not before?
Did you add a test that wasnt there before?

Yes. In amount() the autoFormatted parameter was not tested before. So I added a case for that.

Maybe revert that added test and create a separate issue for that bug.
IMO the bug is unrelated to this change and shouldnt be addressed in the same PR.

@matthewmayer
Copy link
Contributor

calling toLocaleString with an undefined first parameter just seems like a Bad Idea, because it makes the output of Faker dependent on the system locale rather than the Faker locale.

@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #1821 (13e48f9) into next (8f2d5c8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1821      +/-   ##
==========================================
- Coverage   99.63%   99.63%   -0.01%     
==========================================
  Files        2346     2346              
  Lines      235735   236394     +659     
  Branches     1141     1143       +2     
==========================================
+ Hits       234869   235524     +655     
- Misses        844      848       +4     
  Partials       22       22              
Impacted Files Coverage Δ
src/modules/finance/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 88.75% <0.00%> (-2.08%) ⬇️
src/modules/location/index.ts 98.91% <0.00%> (+0.21%) ⬆️
src/modules/system/index.ts 100.00% <0.00%> (+0.25%) ⬆️

@xDivisionByZerox xDivisionByZerox requested a review from a team February 14, 2023 10:47
@ST-DDT ST-DDT enabled auto-merge (squash) February 15, 2023 09:08
@ST-DDT ST-DDT merged commit 1399375 into next Feb 15, 2023
@xDivisionByZerox xDivisionByZerox deleted the refactor/finance/standardize-arguments branch February 15, 2023 16:11
matthewmayer pushed a commit to matthewmayer/faker that referenced this pull request Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: finance Something is referring to the finance module p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants