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

Format option for the curency layout #1672

Closed
Joerg-L opened this issue May 19, 2022 · 10 comments · Fixed by #2017
Closed

Format option for the curency layout #1672

Joerg-L opened this issue May 19, 2022 · 10 comments · Fixed by #2017
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Projects

Comments

@Joerg-L
Copy link

Joerg-L commented May 19, 2022

When setting INFRACOST_CURRENCY=EUR the format of the currency values is not correct.

Current Format

  • Example in USD $3,938.10
  • Example in EUR €3,736.11

Expected format

  • Example in EUR: 3.736,11€

To make it as flexible as possible, it would be helpfull do define an addition environment variable to define the logic like

  • EUR: XXX.XXX,00
  • USD: XXX,XXX.00

USD can be the default.


Proposed solution

To provide flexibility in displaying costs in local currency we can introduce a new ENV var INFRACOST_CURRENCY_FORMAT that accepts a template string in format $123,456.7890 similar to Go's date format. Examples:

  • Default USD: $123,456.78
  • EUR in Germany: 123.456,78$
  • Custom format with 3 decimal points and space between currency character and the value: 123,456.789 $

CLI uses go-money package where Currency struct has Decimal, Thousand, Fraction and Template attributes.

It would parse the ENV var and extract related parts to override these attributes for selected currency. If ENV var is not set - don't do anything and use default go-money's behavior. Rules:

  • Thousand character will be located between 123 and 456
  • Decimal - between 456 and 7* (or $ or space if fraction is 0)
  • Fraction - 7 for 1, 78 - 2, 789 - 3, 7890 - 4
  • Template - replacing 123,456.7890 with 1 would result with 1 $, go-money package will replace $ with the proper currency character.

Note for community

Please 👍 if you'd like to upvote this or receive updates.

@vdmgolub vdmgolub added the enhancement New feature or request label May 19, 2022
@vdmgolub
Copy link
Contributor

@Joerg-L Thank you for creating the issue! Indeed, different currencies may have different formats and currently we use a naive approach for simplicity.

I wonder if having a setting for locale like infracost configure set locale de_DE (or via env var) makes more sense. This way CLI could automatically figure out how to represent the numbers (and maybe even text translation in future).

Would you find it more useful instead of manually providing a format template? I can see an extra complexity with parsing this and handling incorrect formats, but maybe I'm missing a use case here.

@Joerg-L
Copy link
Author

Joerg-L commented May 19, 2022

@vdmgolub I'm completely fine with your approach.

Related to text translation:
if that comes any time in future, an option to disable that would make sense too, I would think.

@vdmgolub
Copy link
Contributor

Oh! That's right, I guess many teams in engineering would use English language for communication. But it seems like you would still prefer to see the costs in your local currency AND format. Very interesting! :) I'll bring this topic to the team to discuss what would work better in a long run.

Thank you for bringing this up! We'll update the issue when we have more details. 👍

@tim775
Copy link
Member

tim775 commented May 19, 2022

We use the go-money package and that already has a template mechanism for positioning the currency symbol. I'm guessing it wouldn't be too difficult to let users set an alternate template.

@vdmgolub
Copy link
Contributor

@tim775 It's not just the symbol position, in Germany comma delimits the decimal part, and dot is a thousands separator. That is why I thought about introducing locale notion.

@vdmgolub vdmgolub added the good first issue Good for newcomers label May 20, 2022
@vdmgolub
Copy link
Contributor

@Joerg-L I've updated the description with a proposed solution. We thought the ENV var would be a good start for now. It's open for community contributions. If you feel like implementing it, please leave a comment and I'll assign it to you. :)

Thanks again for creating the issue! It should be a nice enhancement for the app. :)

@Joerg-L
Copy link
Author

Joerg-L commented May 20, 2022

@vdmgolub thanks for that update.
I have just replace den $ with € in your update....

If I would have any clue in devloping such things, I would like to contribute, to be honset, I don't have a clue.
I'm just an IT guy who is creating terraform configurations.

@vdmgolub
Copy link
Contributor

@Joerg-L No worries at all, was checking in case you're interested to try new things :)

Oops, I need to elaborate it better in the description: the $ is a fixed symbol here, like a placeholder for the currency character. Having a predefined format with known values makes it easy to parse: the parser would know that the thousands delimiter is always between 123 and 456, same for the decimal point - between 456 and 7*. The 7890 would show the count of the fraction size - 4 digits, and $ will highlight where the currency character should be located - go-money package already knows how to replace it with the right one. I'll update the description :)

@fatihtokus
Copy link
Contributor

Hi, I am working on this, please assign it to me.

@vdmgolub vdmgolub added this to Todo (prioritized top to bottom) in Main via automation Sep 28, 2022
@vdmgolub vdmgolub moved this from Todo (prioritized top to bottom) to Merged to master in Main Sep 28, 2022
@hugorut
Copy link
Contributor

hugorut commented Sep 28, 2022

Custom currency and formatting support is now live in the v0.10.12 release. Thanks @fatihtokus for your contribution.

@hugorut hugorut moved this from Merged to master to Released in Main Sep 28, 2022
@hugorut hugorut closed this as completed Sep 28, 2022
Main automation moved this from Released to Merged to master Sep 28, 2022
@tim775 tim775 moved this from Merged to master to Released in Main Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
Main
Released
Development

Successfully merging a pull request may close this issue.

5 participants