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

Switch prices to decimal representation #1

Open
HenningHolmDE opened this issue Jun 7, 2020 · 6 comments
Open

Switch prices to decimal representation #1

HenningHolmDE opened this issue Jun 7, 2020 · 6 comments

Comments

@HenningHolmDE
Copy link
Owner

Currently, prices are repesented as String type, e.g.:

pub struct Price {
/// Price without VAT
#[serde(rename = "net")]
pub net: String,
/// Price with VAT added
#[serde(rename = "gross")]
pub gross: String,
}

Using decimal representation (e.g. rust_decimal) would be more convenient for calculations.

@good-praxis
Copy link
Contributor

Correct me if I'm wrong, but this would also have to be done, essentially, through generate_api_code.sh, right?

@HenningHolmDE
Copy link
Owner Author

Actually, I did not yet thought this through in any way yet.

I only noticed this when I was writing the find_cheapest_server_type example, where I parsed the priced into f32 for comparison.

Changing the implementation after generation would be one way to go, another would probably be starting to provide manually implemented helper functions or structs that handle the conversion. There are probably other ways as well.

@kpcyrd
Copy link

kpcyrd commented May 21, 2022

Would it make sense to use u64 and cents instead to avoid floating point issues?

@MaximilianKoestler
Copy link
Collaborator

Hi, Max (the maintainer of https://github.com/MaximilianKoestler/hcloud-openapi) here.

Going for cents might be tempting but:

  • The price resolution is not fixed to cents to my knowledge, there is nothing keeping Hetzner from pricing things in steps of 0.0001 € if that makes economical sense to them.
  • The core of hcloud-rust stays close to the openAPI document by design to avoid surprises for developers switching from an API binding of a different language to Rust. "Arbitrarily" (not really, but kind of) changing the unit of something somewhere is a recipe for subtle bugs. Adding nicer wrappers on top is fine but I would argue against replacing the original API call.

@HenningHolmDE
Copy link
Owner Author

@kpcyrd I'm actually not quite sure what floating point issues you are referring to. Currently, the API just forwards Hetzner's representation of prices as a string type. For the example I mentioned earlier, floating point errors are not really an issue, because it is just about sorting by price (which would not work for strings). But I would agree with you that using floating point types on the API for representing currency would be a bad idea.

And @MaximilianKoestler is totally right, in fact, the hourly prices for most of the servers are already more precise than one cent, as you can see on the cx11 server type:

PricePerTime {
    location: "hel1",
    price_hourly: Price {
        gross: "0.0065450000000000",
        net: "0.0055000000",
    },
    price_monthly: Price {
        gross: "4.1531000000000000",
        net: "3.4900000000",
    },
},

However, while the strings are the exact representation, they are not really useful for any sort of calculation. This is where my initial thought of providing something more convenient (but in any case still correct) was coming from.

Writing this, I would even consider it more likely that users who are unaware of the "floating point numbers are not a good idea for currency" issue will convert the prices to floating point for calculations when they are only available as strings.

@good-praxis
Copy link
Contributor

We could opt to wrap usage of rust_decimal behind a decimal feature, goal of which being to replace the string values with decimal ones in-line for convenience only. How does that sound?

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

No branches or pull requests

4 participants