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

Allows custom Currency Provider through inheritance #94

Closed
wants to merge 1 commit into from

Conversation

tigitz
Copy link

@tigitz tigitz commented Oct 13, 2024

This is a minimal proof of concept for #30 and #61

It uses inheritance as a point of extension for custom logic.

Pros are that existing userland logic will still work as expected while allowing flexibility for more complex use cases.

However extending through composition would be much more robust but would require a profound rewrite of current classes.

Introduce the ability to provide a list of supported currencies that can be different than the hardcoded ISO 4217 ones (e.g. Crypto)
@BenMorel
Copy link
Member

Interesting approach @tigitz!

It uses inheritance as a point of extension for custom logic.

Why extend Currency though, when we could just have a CurrencyInterface? (Currency should probably be renamed to ISOCurrency in this case, for consistency.)

However extending through composition would be much more robust but would require a profound rewrite of current classes.

Can you share the rough idea without writing the code?

@tigitz
Copy link
Author

tigitz commented Oct 13, 2024

Thanks 🙂

Why extend Currency though, when we could just have a CurrencyInterface? (Currency should probably be renamed to ISOCurrency in this case, for consistency.)

You're right, this is part of the "composition" approach I'm suggesting, which would have a more significant impact. I'm in the process of writing a more detailed comment in #30 to explain everything thoroughly. Catch you there 😉

@tigitz
Copy link
Author

tigitz commented Oct 21, 2024

Superseded by #95

@tigitz tigitz closed this Oct 21, 2024
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.

2 participants