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

feat(finance): currency object #1809

Merged
merged 15 commits into from
Feb 26, 2023

Conversation

matthewmayer
Copy link
Contributor

fix #1733

@matthewmayer matthewmayer requested a review from a team as a code owner February 2, 2023 12:35
@matthewmayer matthewmayer mentioned this pull request Feb 2, 2023
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #1809 (a5db4b3) into next (11a5f51) will increase coverage by 0.00%.
The diff coverage is 99.58%.

Additional details and impacted files
@@           Coverage Diff            @@
##             next    #1809    +/-   ##
========================================
  Coverage   99.63%   99.64%            
========================================
  Files        2355     2355            
  Lines      236459   236843   +384     
  Branches     1159     1155     -4     
========================================
+ Hits       235605   235993   +388     
+ Misses        832      828     -4     
  Partials       22       22            
Impacted Files Coverage Δ
src/definitions/finance.ts 0.00% <0.00%> (ø)
src/definitions/index.ts 0.00% <0.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)
src/locales/el/finance/currency.ts 100.00% <100.00%> (ø)
src/locales/en/finance/currency.ts 100.00% <100.00%> (ø)
src/locales/fa/finance/currency.ts 100.00% <100.00%> (ø)
src/locales/fr/finance/currency.ts 100.00% <100.00%> (ø)
src/locales/fr_CH/finance/currency.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 91.12% <0.00%> (-4.74%) ⬇️
... and 2 more

@Shinigami92 Shinigami92 added s: on hold Blocked by something or frozen to avoid conflicts do NOT merge yet Do not merge this PR into the target branch yet s: needs decision Needs team/maintainer decision labels Feb 2, 2023
src/modules/finance/index.ts Show resolved Hide resolved
src/modules/finance/index.ts Show resolved Hide resolved
@ST-DDT ST-DDT added needs rebase There is a merge conflict c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug m: finance Something is referring to the finance module and removed s: on hold Blocked by something or frozen to avoid conflicts do NOT merge yet Do not merge this PR into the target branch yet s: needs decision Needs team/maintainer decision labels Feb 16, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Feb 16, 2023

Team Decision

Change the locale data to contain [{ name, code, symbol }]

@matthewmayer matthewmayer requested a review from a team February 18, 2023 10:48
@matthewmayer matthewmayer force-pushed the feature/currency-object branch 2 times, most recently from 60120b6 to 57fb85b Compare February 18, 2023 11:01
@matthewmayer
Copy link
Contributor Author

Sorry, had some issues with my branch so had to force-push

@matthewmayer
Copy link
Contributor Author

This was my script to transform the definitions:

'use strict';
const fs = require('fs');
const langs = ['en','el', 'fr_CH', 'fr', 'fa'];
const { faker } = require('@faker-js/faker');
for (let lang of langs) {
  let c = faker.locales[lang].finance.currency;
  let newCurr = Object.keys(c).map((k) => {
    return { name: k, code: c[k].code, symbol: c[k].symbol };
  });
  let str = JSON.stringify(newCurr, null, 2);
  fs.writeFileSync(
    `./src/locales/${lang}/finance/currency.ts`,
    `export default ${str}`
  );
}

@matthewmayer matthewmayer marked this pull request as draft February 18, 2023 11:10
@matthewmayer matthewmayer marked this pull request as ready for review February 18, 2023 11:28
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a random seeded test for the currency object.

src/locales/fr_CH/finance/currency.ts Show resolved Hide resolved
src/modules/finance/index.ts Outdated Show resolved Hide resolved
src/modules/finance/index.ts Outdated Show resolved Hide resolved
src/modules/finance/index.ts Outdated Show resolved Hide resolved
src/modules/finance/index.ts Outdated Show resolved Hide resolved
src/modules/finance/index.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor Author

add a test of the currency object

added in 844cb20

@matthewmayer
Copy link
Contributor Author

i think there are a few "currencies" which could be removed e.g.

{
name: 'Codes specifically reserved for testing purposes',
code: 'XTS',
symbol: '',
},

and various metals like Gold, Platinum etc,

Should i include that in this PR or wait and do as a seperate PR

@matthewmayer
Copy link
Contributor Author

i think there are a few "currencies" which could be removed e.g.

{ name: 'Codes specifically reserved for testing purposes', code: 'XTS', symbol: '', },

and various metals like Gold, Platinum etc,

Should i include that in this PR or wait and do as a seperate PR

comparing https://en.wikipedia.org/wiki/List_of_circulating_currencies with the list i'd suggest the removal of

[
  'CUC', 'EEK', 'HRK', 'LTL',
  'LVL', 'MRO', 'SLL', 'SVC',
  'XAG', 'XAU', 'XBA', 'XBB',
  'XBC', 'XBD', 'XDR', 'XFU',
  'XPD', 'XPT', 'XTS', 'ZWL'
]

and the addition of

[ 'MRU', 'SLE', 'SSP'] (and possibly 'VED' )

these are a variety of currencies which got replaced (e.g. EEK -> EUR), changed (e.g. MRO -> MRU) or are non-currencies (e.g. XTS, XAU).

@ST-DDT
Copy link
Member

ST-DDT commented Feb 19, 2023

i think there are a few "currencies" which could be removed e.g.

{ name: 'Codes specifically reserved for testing purposes', code: 'XTS', symbol: '', },

and various metals like Gold, Platinum etc,

Should i include that in this PR or wait and do as a seperate PR

Please do that in a separate PR as the removed entries will be lost within the other changes to the locale definitions.

ST-DDT
ST-DDT previously approved these changes Feb 19, 2023
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the re-export and the deprecation this looks good to me.

@matthewmayer
Copy link
Contributor Author

Other than the re-export and the deprecation this looks good to me.

i added the re-export in 86d1ae0

Not planning to deperecate other methods (per #1809 (comment) ) unless others feel strongly they should be

@Shinigami92 Shinigami92 requested review from ST-DDT and a team February 22, 2023 08:12
@ST-DDT
Copy link
Member

ST-DDT commented Feb 22, 2023

Lets discuss this in the next team meeting.

@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision and removed s: needs decision Needs team/maintainer decision labels Feb 22, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Feb 23, 2023

Team decision

We won't deprecate the old methods.


Then this PR is ready for review and merge.

@ST-DDT ST-DDT requested a review from a team February 24, 2023 22:59
@ST-DDT ST-DDT merged commit 59157a4 into faker-js:next Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: finance Something is referring to the finance module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Better currencies
3 participants