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

Remove hardcoded dependency on default money context #334

Closed

Conversation

fredfp
Copy link

@fredfp fredfp commented Jan 8, 2019

Squants market package is designed to allow defining a different set of currencies, with optional FX rates.

This flexibility is achieved by using an implicit MoneyContext parameter on some methods, with a default value set to squants.market.defaultMoneyContext. This way, one can define a new MoneyContext, and mark it implicit to have it used instead of the default.

This however isn't consistently implemented throughout the code base. It is especially missing on factory methods of the Money object which involve parsing a currency code. As a consequence, one cannot build Money instances using custom currencies (i.e., currencies which are not available in the squants market package).

This pull request is meant to fix this issue. It does so by removing any hard-coded references to the default currency set or default money context, and replacing them by an implicitly passed MoneyContext.

@fredfp
Copy link
Author

fredfp commented Jan 8, 2019

@garyKeorkunian

@fredfp
Copy link
Author

fredfp commented Jan 9, 2019

I missed #330 before creating this PR.

This one is slightly different: it keeps the current squants behaviour (i.e., crashing for missing currencies) which is a rather bad thing but was meant to trigger a conversation in here.

@cquiroz could you advise on which way to go to get one of those merged and released?

should fix #296

@cquiroz
Copy link
Collaborator

cquiroz commented Jan 19, 2019

I'm a bit confused abou these. So #334 and #330 Do the same?

@fredfp
Copy link
Author

fredfp commented Jan 19, 2019

They do the same except when parsing a Currency from a String:

I'd prefer having #330 merged

@garyKeorkunian
Copy link
Contributor

#330 has been approved and will be merged soon.

@fredfp fredfp closed this Feb 12, 2019
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