Proof of concept: Use a trait for parsers#529
Proof of concept: Use a trait for parsers#529gmponos wants to merge 14 commits intomoneyphp:masterfrom
Conversation
…hat most probably a trait will be useful
gmponos
force-pushed
the
proof_of_concept_use_trait
branch
from
7debca6 to
90d52eb
Compare
|
@gmponos Any updates? |
|
This could be my fault.. but I need some feedback in order to proceed and finalize this or not. Neither on the current PR or here I got any feedback. |
|
@gmponos This is look very nice for me, but I am not scure, why we can not use |
|
|
||
| trait DecimalParserTrait | ||
| { | ||
| public static $decimalPattern = '/^(?P<sign>-)?(?P<digits>0|[1-9]\d*)?\.?(?P<fraction>\d+)?$/'; |
There was a problem hiding this comment.
This should be a const IMHO
There was a problem hiding this comment.
Traits can not have constants :)
There was a problem hiding this comment.
In that case, I would make it protected.
| */ | ||
| final class DecimalMoneyParser implements MoneyParser | ||
| { | ||
| const DECIMAL_PATTERN = '/^(?P<sign>-)?(?P<digits>0|[1-9]\d*)?\.?(?P<fraction>\d+)?$/'; |
There was a problem hiding this comment.
Note that removing this constant is probably a BC break.
|
Like the idea! Can you please rebase the branch from master? |
Co-Authored-By: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com>
gmponos
changed the title
|
Marked as WIP.. let's merge this first: #586 |
|
Sorry @gmponos, I think we are going to leave this as is. In my opinion traits are to be avoided. |
4 participants
Made all the parsers use the same code by using a trait..
At my initial commits I made all the parsers use the same code. And then later I replaced the code with a trait.
@frederikbosch made a comment here
I had also thought about the same idea but later on my plan was to submit a PR just like this one.
Marking this trait as internal will also not be inside the BC promise.