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

Added support for advanced currency management orgs (also known as dated exchange rate) #583

Closed
wants to merge 4 commits into from

Conversation

Kilay
Copy link

@Kilay Kilay commented Nov 7, 2017

Added support for advanced currency management orgs (issue #82 )

@afawcett
Copy link
Collaborator

@Kilay thanks so much for this, this looks good. My only concern is the amount of diffs on the LREngine class. This PR is also missing the text class the CurrencyManagement.cls (may need to remove references to NexthinkProvider also?). I thin added a new operation to perform the query aggregation in memory is the best fix. So if you can clean up the change it would be a most welcome feature to the tool, for some this is a real issue. 👍

@afawcett
Copy link
Collaborator

Plus one from me, adding a new operation type seems the best way to go, not everyone has this issue with currencies and the cost of doing the aggregation in memory is something that needs the developer to opt-in if they need this behaviour. You may want to revise the test class to remove references to NexthinkProvider.

@Kilay
Copy link
Author

Kilay commented Nov 30, 2017

I created a new PR (#591) in order to reduce amount of change in LREngine class.

@Kilay Kilay closed this Nov 30, 2017
@afawcett
Copy link
Collaborator

afawcett commented Dec 7, 2017

Thanks @Kilay, awesome work!

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