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

Add (failing) test case for allocateWithRemainder #55

Closed

Conversation

artfulrobot
Copy link

Given ratios 75, 25, and an amount of 0.03 GBP, I would expect Brick not to allocate any money, since it cannot be divided according to the ratios.

i.e. I'd expect: 0, 0, remainder 0.03.

Instead it allocates 0.02, 0, remainder 0.01

Therefore successive allocations of small amounts would result in all the money getting funnelled to the first account, which the owner of the 2nd account might not feel is fair.

@artfulrobot
Copy link
Author

artfulrobot commented Dec 15, 2021

Theoretically, I think Brick should refuse to allocate when the lowest common denominator of the ratio (4 in this case) is greater than the number of minor units to allocate (3 in this case).

However I've just noticed this test case https://github.com/brick/money/blob/0.5.3/tests/MoneyTest.php#L437
Where you say that ratios 1, 1, 3, 1 (i.e. 1/6ths and 1/2) applied to 0.02 should result in nothing for the sixths and 0.01 for the half.

This seems wrong to me.

@BenMorel
Copy link
Member

@NCatalani Do you want to fix this?

@BenMorel
Copy link
Member

BenMorel commented Jun 19, 2022

I have a working fix at #62. Ignoring the GCD calculations part that should be moved to brick/math, could you please review the implementation and tests, @artfulrobot @NCatalani?

Also, do you think we should change the implementation of allocate() as well?

@BenMorel
Copy link
Member

Ping @artfulrobot @NCatalani

@artfulrobot
Copy link
Author

@BenMorel I'm happy to take a look, but I'm v busy at mo. I've put it on my do-list though.

@BenMorel BenMorel closed this in #62 Aug 1, 2022
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