[Yaml] Allow using _ in some numeric notations #18486
Conversation
9ba1c17 to
4597c12
Compare
|
👍 |
| @@ -564,6 +565,8 @@ private static function evaluateScalar($scalar, $flags, $references = array()) | |||
| return; | |||
| case 0 === strpos($scalar, '!!float '): | |||
| return (float) substr($scalar, 8); | |||
| case preg_match('{^[+-]?[0-9][0-9_]*$}', $scalar): | |||
| $scalar = str_replace('_', '', (string) $scalar); | |||
There was a problem hiding this comment.
Please add a comment saying that the fallthrough is intentional, to avoid mistakes in the future
There was a problem hiding this comment.
will do as soon as i get my hands on my dev env
|
👍 @nicolas-grekas could you check why a test is failing when removing a folder ? |
|
@stof this issue has already been fixed a few days ago hopefully |
4597c12 to
0f12481
Compare
|
👍 |
|
Should be merged when master becomes 3.2. |
| @@ -584,8 +590,8 @@ private static function evaluateScalar($scalar, $flags, $references = array()) | |||
| return log(0); | |||
| case 0 === strpos($scalar, '!!binary '): | |||
| return self::evaluateBinaryScalar(substr($scalar, 9)); | |||
| case preg_match('/^(-|\+)?[0-9,]+(\.[0-9]+)?$/', $scalar): | |||
| return (float) str_replace(',', '', $scalar); | |||
| case preg_match('/^(-|\+)?[0-9][0-9,_]*(\.[0-9_]+)?$/', $scalar): | |||
There was a problem hiding this comment.
This will support numbers mixing , and _. I am not convinced we should support that.
There was a problem hiding this comment.
IMO, removing , (which is not in the yaml spec AFAIK) would be a bc break, so that's why I kept it. Adding a deprecation is also not within the scope of this PR IMO
There was a problem hiding this comment.
I agree that the deprecation should be added in a separate PR. But imo we should only support numbers here that contain either commas or underscores:
case preg_match('/^(-|\+)?[0-9][0-9,]*(\.[0-9]+)?$/', $scalar):
case preg_match('/^(-|\+)?[0-9][0-9_]*(\.[0-9_]+)?$/', $scalar):
// ...In a new PR we can then add the deprecation trigger after the first case.
There was a problem hiding this comment.
OK, I can indeed split this case into two cases
There was a problem hiding this comment.
If you're referring to float, isn't it 1_200.2_200 ? Looking at http://yaml.org/type/float.html, in the regexp there is no , in fact (while there is a _)
There was a problem hiding this comment.
So yes, we are keeping that, it is just the , that should be deprecated as it's nowhere in the yaml specs
There was a problem hiding this comment.
This case was split as suggested
0f12481 to
e6da11c
Compare
|
👍 |
|
Thank you @Taluu. |
This PR was merged into the 3.2-dev branch. Discussion ---------- [Yaml] Allow using _ in some numeric notations | Q | A | ------------- | --- | Branch | master | Bug fix? | no ? | New feature? | yes ? | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18094 | License | MIT | Doc PR | ~ Allows to use the `_` to group "big" ints, as suggested in the yaml integer type specification. As discussed in #18094, we should check if it is still part of the 1.2 specification, but I don't really see why not ? I can't see anywhere anything saying it is not valid anymore... as there are links to these types in some other specs. This is #18096, but targetted on master as this is considered as a new feature. I also support the dump of such values as only strings. I think I should change how it is dumped thoug, and use the escape filter instead though (as I was misusing the data provider and it provided strange results at the time) Commits ------- e6da11c [Yaml] Allow using _ in some numeric notations
Allows to use the
_to group "big" ints, as suggested in the yaml integer type specification. As discussed in #18094, we should check if it is still part of the 1.2 specification, but I don't really see why not ? I can't see anywhere anything saying it is not valid anymore... as there are links to these types in some other specs.This is #18096, but targetted on master as this is considered as a new feature. I also support the dump of such values as only strings. I think I should change how it is dumped thoug, and use the escape filter instead though (as I was misusing the data provider and it provided strange results at the time)