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(postgres): fix processing money type #339

Merged
merged 3 commits into from
Feb 8, 2017
Merged

fix(postgres): fix processing money type #339

merged 3 commits into from
Feb 8, 2017

Conversation

benjie
Copy link
Member

@benjie benjie commented Feb 1, 2017

Finally got around to figuring out what was wrong with the tests for me...

I'm concerned that the removal of commas might be localization dependent too, for example in continental Europe they tend to place commas where we place dots and vice versa; e.g. €1.234,56 - if this is the case for the money type then things are going to go wrong... 😜

@benjie benjie requested a review from calebmer February 1, 2017 22:48
@calebmer
Copy link
Collaborator

calebmer commented Feb 1, 2017

Do you think it would be worth using something like https://github.com/openexchangerates/accounting.js for parsing monetary formats? Because you’re right about the localization thing 😉

@benjie
Copy link
Member Author

benjie commented Feb 2, 2017

Seems overkill; could we just cast the column to float or decimal when selecting?

@benjie
Copy link
Member Author

benjie commented Feb 5, 2017

@calebmer Should we merge this as-is and revisit accounting.js at a later date? I'm just aware that currently the money type doesn't work outside the US for anyone...

@calebmer
Copy link
Collaborator

calebmer commented Feb 7, 2017

Yep 👍

Just waiting for CI to pass now…

@benjie benjie merged commit 2f25ce8 into master Feb 8, 2017
@benjie benjie deleted the fix-money branch February 8, 2017 00:43
Belline pushed a commit to Belline/postgraphql that referenced this pull request Dec 18, 2017
benjie pushed a commit that referenced this pull request Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants