-
Notifications
You must be signed in to change notification settings - Fork 441
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
Parse exponential float as Money #520
Conversation
@money-team let me know if this going to the correct direction for you in order to finalize the PR and fix the CI failures. |
@gmponos Why not inject a Decimal parser and then delegate the parsing process once you have handled the exponential part? Maybe I should have a closer look, but as it seems to me there is a lot of duplicate code? public function __construct(Currencies $currencies, DecimalMoneyParser $delegatedParser) {
$this->currencies = $currencies;
$this->delegatedParser = $delegatedParser;
}
public function parse($money, $forceCurrency = null) {
// ....
$number = number_format($expo, $subunit, '.', '');
return $this->delegatedParser->parse($number, $forceCurrency);
} |
@frederikbosch I though about the same solution too. But I dislike it for the following reasons:
ExponentialParser.php
The same checks above will be checked again inside the
Anyway my current solution stands to the following facts:
|
A trait indeed seems to be an easier solution in this case as the delegated parser would always be a decimal parser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Should wait for the trait though
Co-Authored-By: Márk Sági-Kazár <[email protected]>
|
||
if (null === $forceCurrency) { | ||
throw new ParserException( | ||
'DecimalMoneyParser cannot parse currency symbols. Use forceCurrency argument' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this say "ExponentialMoneyParser", not "DecimalMoneyParser"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.. nice catch
@gmponos Could you update this PR against the latest master? Sorry for our very late action on this. |
Superseded by #668. |
Created a new Parser that will be able to parse exponential values to Money object. This solves the issue that I have presented on this PR #494
This parser is used only for exponential values and not for decimals.. It will be good to be used combined with an Aggregator parser and a decimal parser.