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

Better currencies #1733

Closed
kg-currenxie opened this issue Jan 13, 2023 · 9 comments · Fixed by #1809
Closed

Better currencies #1733

kg-currenxie opened this issue Jan 13, 2023 · 9 comments · Fixed by #1809
Assignees
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 s: accepted Accepted feature / Confirmed bug

Comments

@kg-currenxie
Copy link

Clear and concise description of the problem

This will have mismatched data :)

supportedCurrency: {
  code: faker.finance.currencyCode(),
  name: faker.finance.currencyName()
}

Suggested solution

Like unit(), it returns { name: 'meter', symbol: 'm' }

It would be nice if finance.currency() returned { name: 'US Dollar', symbol: '$', code: 'USD' }

Alternative

No response

Additional context

No response

@kg-currenxie kg-currenxie added the s: pending triage Pending Triage label Jan 13, 2023
@ejcheng ejcheng added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision 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 and removed s: pending triage Pending Triage labels Jan 13, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Jan 13, 2023

We are currently in a debate whether or not we should provide these complex objects (airport/airline...).

Could you give us an example why it is important that you get matching currency names and symbols instead of any?

Also could you think of a general rule what should be provided in these complex objects and what shouldnt?
Otherwise we might end up with something like food: ( name, description, ingredients and their amount, calories, ...)

@ST-DDT
Copy link
Member

ST-DDT commented Jan 14, 2023

From the airline PR: #1699 (comment)

For me right now, it isn't important. So I'm good leaving the data the way it is now or splitting it up into separate airline.name: string[] and airline.iataCode: string[]` functions.

@kg-currenxie
Copy link
Author

My use case: I'm mocking an API response, and I'd like to show currencies in my UI in a dropdown, like so USD (Dollar). But I'd get EUR (Indian Rupee) 😂

@matthewmayer
Copy link
Contributor

i think in general complex objects make sense if the objects reflect real-world things and so there's a fixed mapping between values.

For example

ChemicalElement {code:"Au", name: "Gold"}
Airline {name:"British Airways", iataCode:"BA"}
Currency {code:"EUR", name:"Euro"}

but NOT when the values are themselves fake/arbitrary

Dish {name:"Fish and Chips", description: "A delicious flaky dish of cod and potatoes", calories: 1234}
Pet {name:"Fluffy", type:"Dog", color:"Black"}

@ejcheng
Copy link
Member

ejcheng commented Jan 14, 2023

i think in general complex objects make sense if the objects reflect real-world things and so there's a fixed mapping between values.

For example

ChemicalElement {code:"Au", name: "Gold"}
Airline {name:"British Airways", iataCode:"BA"}
Currency {code:"EUR", name:"Euro"}

but NOT when the values are themselves fake/arbitrary

Dish {name:"Fish and Chips", description: "A delicious flaky dish of cod and potatoes", calories: 1234}
Pet {name:"Fluffy", type:"Dog", color:"Black"}

This makes sense, it would be weird to create an object:

{ name: faker.airline.airlineName(), iataCode: faker.airline.iataCode() }

and get `{ name: "Icelandair", iataCode: "BA" }

@matthewmayer
Copy link
Contributor

since the airline PR landed, and since this seems to follow the rule of thumb i wrote at #1733 (comment) i went ahead and added a PR at #1809

@Shinigami92
Copy link
Member

I'm sorry you missed #1699 (comment)

@matthewmayer
Copy link
Contributor

Okay no worries just leave this unmerged until you make a final decision about how to handle these kind of issues.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 16, 2023

Team decision

We will support complex objects for static data (such as currency), but we won't add dynamic complex objects (such as a person{firstName, lastName}).

We have to add support for the fake method (#1850), deprecate the individual currencyName/code methods and remove datatype.object and array (Separate issues/PRs).

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Feb 16, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Feb 16, 2023
@ST-DDT ST-DDT moved this to In Progress in Faker Roadmap Feb 16, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Faker Roadmap Feb 26, 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 s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants