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

Parse exponential float as Money #520

Closed
wants to merge 7 commits into from
Closed

Conversation

gmponos
Copy link
Contributor

@gmponos gmponos commented Dec 2, 2018

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.

@gmponos
Copy link
Contributor Author

gmponos commented Dec 2, 2018

@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.

@frederikbosch
Copy link
Member

@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);
}

@gmponos
Copy link
Contributor Author

gmponos commented Dec 5, 2018

@frederikbosch I though about the same solution too. But I dislike it for the following reasons:

  1. First of all I had in mind some next steps that can be taken. It can be found here. The argument about duplication stands almost for every parser. So the next step could be using a trait in all parsers and avoiding the duplication.
  2. Secondly I will need to do some checks inside exponential parser and then the same checks will be repeated inside the decimal parser. For instance:

ExponentialParser.php

    public function parse($money, $forceCurrency = null)
    {
        if (!is_string($money)) {
            throw new ParserException('Formatted raw money should be string, e.g. 1.00');
        }

        if (null === $forceCurrency) {
            throw new ParserException(
                'ExponentialParser cannot parse currency symbols. Use forceCurrency argument'
            );
        }
       ....
      $this->delegatedParser->parse(...);

The same checks above will be checked again inside the delegatedParser

  1. The initialization process in order to add inside an AggregatorParser a DecimalParser and an ExponentialParser it would feel kinda odd for me...
$decimal = new DecimalParser(...);
$exponention = new ExponentialParser($decimal, $currencies);
$aggregator = new AggregatorParser([
    $decimal, 
    $exponential
])

Anyway my current solution stands to the following facts:

  1. Most of the parsers already have the same code.. written a little bit differently.. check the PR above.
  2. I could implement the solution directly inside DecimalParser but my solution requires to use number_format function. if I did that most probably it would raise some questions like why the DecimalParser doesn't use the number_format function on the rest of the code. And also are values like these "1.234e-11" considered "Decimals".. Playing with words here...

@sagikazarmark sagikazarmark modified the milestones: 3.3.0, 3.4.0 Dec 27, 2019
@sagikazarmark
Copy link
Collaborator

A trait indeed seems to be an easier solution in this case as the delegated parser would always be a decimal parser.

Copy link
Collaborator

@sagikazarmark sagikazarmark left a 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

src/Parser/ExponentialMoneyParser.php Outdated Show resolved Hide resolved

if (null === $forceCurrency) {
throw new ParserException(
'DecimalMoneyParser cannot parse currency symbols. Use forceCurrency argument'

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.. nice catch

@frederikbosch
Copy link
Member

@gmponos Could you update this PR against the latest master? Sorry for our very late action on this.

@frederikbosch
Copy link
Member

Superseded by #668.

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.

None yet

4 participants