-
Notifications
You must be signed in to change notification settings - Fork 6
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
T110728: Trim trailing newlines from DecimalValues #58
Conversation
$a = $this->getValue(); | ||
$b = $that->getValue(); | ||
$a = $this->value; | ||
$b = $that->value; |
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 kind of "btw" change makes review harder for me.
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.
I worked extra hard to minimize the diff to an absolute minimum. I could have uploaded these 3 lines as a separate patch, and do this most of the time, but found its not worth it in this case.
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.
But why change this at all?
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.
Why polluting the stack trace to access a property?
Seems fine overall, though I don't like the "marginal" changes much. |
@@ -40,46 +40,38 @@ class DecimalValue extends DataValueObject { | |||
* Regular expression for matching decimal strings that conform to the format | |||
* described in the class level documentation of @see DecimalValue. | |||
*/ | |||
const QUANTITY_VALUE_PATTERN = '/^[-+]([1-9]\d*|\d)(\.\d+)?$/'; | |||
const QUANTITY_VALUE_PATTERN = '/^[-+]([1-9]\d*|\d)(\.\d+)?\z/'; |
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.
/^-+(.\d+)?$/D should also work.
This will make bogus QuantityValues in the database magically disappear over time, every time an entity is edited. The newlines will also immediately disappear from all exports.
Bug: T110728