Skip to content

Add Money.with_bank for Dynamic Currency Stores#713

Closed
pranavbabu wants to merge 6 commits intoRubyMoney:mainfrom
MaindeckAS:main
Closed

Add Money.with_bank for Dynamic Currency Stores#713
pranavbabu wants to merge 6 commits intoRubyMoney:mainfrom
MaindeckAS:main

Conversation

@pranavbabu
Copy link

This PR introduces the Money.with_bank method, allowing developers to assign different currency stores (banks) dynamically within a scoped block. This is particularly useful for loading custom exchange rates per user session.

Key Changes

  • Implemented Money.with_bank(bank_instance), enabling temporary overrides of the default bank.
  • Ensures thread safety by restoring the original bank after execution.
  • Added documentation and an example demonstrating its usage in a Rails controller with around_action.

@pranavbabu pranavbabu changed the title Enhancement: Add Money.with_bank for Dynamic Currency Stores Add Money.with_bank for Dynamic Currency Stores Feb 25, 2025
@yukideluxe
Copy link
Member

yukideluxe commented Jul 9, 2025

@pranavbabu would you mind rebasing your branch with main? We've fixed CI! 😊

@pranavbabu
Copy link
Author

@pranavbabu would you mind rebasing your branch with main? We've fixed CI! 😊

Github did merge automatically when I synced my fork ;)

Copy link
Member

@sunny sunny left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Member

@yukideluxe yukideluxe left a comment

Choose a reason for hiding this comment

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

Hello @pranavbabu, thanks for your contribution! I have one question and one concern about this 😊

  • Why did you add this change to money-rails instead of to money? I have a feeling this isn’t Rails-specific, so it seems like it might belong in the money gem instead, but I might be missing something, I’m still learning!
  • I already had some concerns about this, but after reviewing RubyMoney/money#1128, I’m almost certain this solution won’t be thread-safe. I think it should be implemented similarly to how Money.with_rounding_mode is done using Thread.current. Could you maybe add some tests like the ones here? 🙏🏻

@pranavbabu
Copy link
Author

Hey @yukideluxe, I added it to this lib just because we had this fix in our company fork for a very long time , I've never thought about money gem itself since we use rails 😅 so you might be correct and I should address it in another gem, your second point is actually really useful, we had some weird bugs where bank was not set and we had currency exchange problems , so now I know why 🙈 big thanks , I'm gonna close this one in favour of new PR to money gem , after vacation 🤗

@yukideluxe
Copy link
Member

Hey @yukideluxe, I added it to this lib just because we had this fix in our company fork for a very long time , I've never thought about money gem itself since we use rails 😅 so you might be correct and I should address it in another gem, your second point is actually really useful, we had some weird bugs where bank was not set and we had currency exchange problems , so now I know why 🙈 big thanks , I'm gonna close this one in favour of new PR to money gem , after vacation 🤗

Great! thanks! I'll keep an eye for your PR in the other repo 👀

@pranavbabu
Copy link
Author

Closing in favour of #this PR

@pranavbabu pranavbabu closed this Jul 14, 2025
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