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

fix(gtfs.yml): add more currency types for fares #551

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

landonreed
Copy link
Contributor

@landonreed landonreed commented Apr 6, 2020

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

fixes #550 by adding top currencies traded in world (and major North American currency types).

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

We should add all possible currency types while we're at it. I think it'd be useful to create some kind of script that can automatically output all of these values. Perhaps we can use the currency-codes package as a data source.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Apr 6, 2020
@landonreed
Copy link
Contributor Author

Yea, perhaps you're right @evansiroky.

@binh-dam-ibigroup
Copy link
Contributor

Instead of keying them in manually, can that be retrieved from some other source?

@landonreed
Copy link
Contributor Author

landonreed commented Apr 14, 2020

Actually, I'm not really sure including all of them is warranted @evansiroky and @binh-dam-ibigroup. Having hundreds of values really just clutters the dropdown immensely. Sure, specifying some obscure currency is valid, but if our primary users of the system are North American transit agencies, does it really help to have "Platinum" as a currency type in here?

@landonreed landonreed assigned evansiroky and unassigned landonreed Apr 14, 2020
@binh-dam-ibigroup
Copy link
Contributor

I agree with @landonreed, it's fine to key in a few like in this PR, but it doesn't make sense to key in everything, especially if we just copy from some other place.

@evansiroky
Copy link
Contributor

evansiroky commented Apr 21, 2020

I'm still a fan of allowing all possible values. The GTFS spec is clear that any ISO 4217 currency code is a valid value. Since we're making software that produces GTFS feeds I don't think it makes sense to exclude valid possible values especially when most values can easily be hidden in a react-select combobox. Go ahead and overrule me if you like though.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Apr 21, 2020
@landonreed
Copy link
Contributor Author

landonreed commented Apr 24, 2020

@evansiroky, how do you propose we hide the values with react-select? I'm open to the idea. Feel free to jump on and add commits to #566.

@landonreed landonreed mentioned this pull request Apr 28, 2020
5 tasks
@evansiroky evansiroky added BLOCKED Blocked (waiting on another PR to be merged) and removed BLOCKED Blocked (waiting on another PR to be merged) labels Jun 9, 2020
Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

Approving for now, but #566 is on my backburner and should be implemented eventually.

@landonreed landonreed merged commit 646dfcf into dev Aug 10, 2020
@landonreed landonreed deleted the add-more-currencies branch August 10, 2020 21:04
@landonreed landonreed mentioned this pull request Aug 12, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLOCKED Blocked (waiting on another PR to be merged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more currencies for fares
3 participants