-
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
Proof of concept: Use a trait for parsers #529
Conversation
…hat most probably a trait will be useful
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 |
src/Parser/DecimalParserTrait.php
Outdated
|
||
trait DecimalParserTrait | ||
{ | ||
public static $decimalPattern = '/^(?P<sign>-)?(?P<digits>0|[1-9]\d*)?\.?(?P<fraction>\d+)?$/'; |
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.
This should be a const IMHO
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.
Traits can not have constants :)
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.
In that case, I would make it protected.
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.
@@ -16,7 +14,7 @@ | |||
*/ | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that removing this constant is probably a BC break.
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.
reverted
Like the idea! Can you please rebase the branch from master? |
Co-Authored-By: Márk Sági-Kazár <[email protected]>
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. |
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.