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

Money::allocate() fixed bug when Money was negative #14

Merged
merged 1 commit into from
Dec 1, 2017

Conversation

trebi
Copy link
Contributor

@trebi trebi commented Dec 1, 2017

No description provided.

src/Money.php Outdated
@@ -537,6 +540,8 @@ public function allocate(int ...$ratios) : array
$remainder = $remainder->minus($unit);
}

Copy link
Member

Choose a reason for hiding this comment

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

No assertions please, I don't use them anywhere else in this library, and test cases are expected to catch the bugs (a test case for negative numbers was missing though, as you found out! 👍 )

src/Money.php Outdated
@@ -519,6 +519,9 @@ public function allocate(int ...$ratios) : array

$unit = BigDecimal::ofUnscaledValue($step, $this->amount->getScale());
$unit = new Money($unit, $this->currency, $this->context);
if ($this->isNegative()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a newline above this one, this keeps the code easier to read.

@BenMorel
Copy link
Member

BenMorel commented Dec 1, 2017

Good catch, thanks! 👍
Once you've changed the two little things above, I'll merge it and release a patch version.

@trebi
Copy link
Contributor Author

trebi commented Dec 1, 2017

✔️ changed

@BenMorel BenMorel merged commit 178f4ba into brick:master Dec 1, 2017
@BenMorel
Copy link
Member

BenMorel commented Dec 1, 2017

Thanks! Released as 0.2.3.

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