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

Add euro format #3850

Merged
merged 13 commits into from
Jan 16, 2024
Merged

Add euro format #3850

merged 13 commits into from
Jan 16, 2024

Conversation

bcolloran
Copy link
Contributor

continuing the work started by @Fredehagelund92 in PR #3748

@bcolloran bcolloran requested a review from ericpgreen2 January 15, 2024 23:36
@bcolloran bcolloran self-assigned this Jan 15, 2024
@bcolloran bcolloran mentioned this pull request Jan 15, 2024
// [789.1234686 / 100, "789.12%"], // ACTUALLY GETTING '789.1%'
// [89.1234686 / 100, "89.12%"], // ACTUALLY GETTING '89.1%'
// [9.1234686 / 100, "9.12%"], // ACTUALLY GETTING '9.1%'
// [0.1234686 / 100, "0.12%"], // ACTUALLY GETTING '0.1%'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericpgreen2 -- note that this has been entirely disabled for months. I've commented out individual cases that are not working exactly as desired (though they are small edge cases that have not been bothersome in practice), but I reckon it's better to have most of the cases running and these ones commented out with FIXME than to turn off the whole file

@bcolloran
Copy link
Contributor Author

@ericpgreen2 This PR adds a currency_eur format preset, and does some amount of the cleanup we talked about the other day without going too deep -- I can revisit if there are product needs that would point us in that direction.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Nice, thanks @Fredehagelund92 for getting this started, and thanks @bcolloran for getting it over-the-line!

As @bcolloran and I discussed, this is a short-path to add the Euro. When adding many more currencies, we should think about a more scalable strategy/syntax.

@bcolloran bcolloran merged commit 6e5c22c into main Jan 16, 2024
5 checks passed
@bcolloran bcolloran deleted the add-euro-format branch January 16, 2024 17:43
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.

3 participants