Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Yaml] Allow using _ in some numeric notations#18486
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9ba1c17 to4597c12Comparefabpot commentedApr 15, 2016
👍 |
| case0 ===strpos($scalar,'!!float'): | ||
| return (float)substr($scalar,8); | ||
| casepreg_match('{^[+-]?[0-9][0-9_]*$}',$scalar): | ||
| $scalar =str_replace('_','', (string)$scalar); |
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.
Please add a comment saying that the fallthrough is intentional, to avoid mistakes in the future
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.
will do as soon as i get my hands on my dev env
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
stof commentedApr 15, 2016
👍 @nicolas-grekas could you check why a test is failing when removing a folder ? |
nicolas-grekas commentedApr 15, 2016
@stof this issue has already been fixed a few days ago hopefully |
dunglas commentedApr 19, 2016
👍 |
fabpot commentedApr 20, 2016
Should be merged when master becomes 3.2. |
| returnself::evaluateBinaryScalar(substr($scalar,9)); | ||
| casepreg_match('/^(-|\+)?[0-9,]+(\.[0-9]+)?$/',$scalar): | ||
| return (float)str_replace(',','',$scalar); | ||
| casepreg_match('/^(-|\+)?[0-9][0-9,_]*(\.[0-9_]+)?$/',$scalar): |
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 will support numbers mixing, and_. I am not convinced we should support that.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
casepreg_match('/^(-|\+)?[0-9][0-9,]*(\.[0-9]+)?$/',$scalar):casepreg_match('/^(-|\+)?[0-9][0-9_]*(\.[0-9_]+)?$/',$scalar):// ...
In a new PR we can then add the deprecation trigger after the firstcase.
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.
OK, I can indeed split this case into two cases
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? Isn't1_200,2_200 valid?
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.
If you're referring to float, isn't it1_200.2_200 ? Looking athttp://yaml.org/type/float.html, in the regexp there is no, in fact (while there is a_)
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.
Sorry, I meant1_200.2_200
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This case was split as suggested
xabbuh commentedApr 22, 2016
👍 |
xabbuh commentedMay 14, 2016
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)