search

varunsrin / rusty_money

Money library for Rust
MIT License
86 stars 32 forks source link

Account for inverse rate #52

Closed JavedNissar closed 7 months ago

JavedNissar commented 3 years ago

This is a PR to address #51. Feel free to close.

JavedNissar commented 3 years ago

First off, thanks for submitting this, appreciate the PR!

Meta thoughts on this:

  1. What is the primary benefit of doing the ExchangeRateQuery over just building the logic into the Exchange in your opinion? I'm trying to justify the added complexity of another class here.
  2. On further reflection, forcing the inverse rate to always be set isn't desirable for all cases (e.g. currency spreads). I think this should be an optional flag on set_rate.
  3. Since this is a big API change, ill hold this back till the 0.5.0 release if we do decide to go forward with it.

You're welcome! Thanks for publishing this library, it's great! With regard to those meta thoughts:

  1. I think I covered this in a reply to one of your line comments
  2. Also in a line comment reply
  3. That's totally fine!
JavedNissar commented 3 years ago

@varunsrin Are you still interesting in having this PR merged in? If not, I can close it.