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#18096
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
Taluu commentedMar 10, 2016
It seems that binaries too should accept this, but as on 2.3 it is not handled (maybe on master though ? as the binary notation was introduced on php 5.4 |
nicolas-grekas commentedMar 15, 2016
Leading |
Taluu commentedMar 15, 2016
Indeed. I'll fix that then. |
| return -log(0); | ||
| case'-.inf' ===$scalarLower: | ||
| returnlog(0); | ||
| casepreg_match('/^(-|\+)?[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.
I don't think I saw anywhere that the, was authorized though ? I kept it, but nothing in the specs says it should be used as a separator (And as@nicolas-grekas noticed,_123_456_ should not be a valid number, so shouldn't,123,456, be also considered invalid or a string ?
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.
You should check with a reference parser, e.g.http://yaml-online-parser.appspot.com/
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.
That tool is awesome, and yes, the, is not allowed as a separator (it renders as string when one is used in a number). I'd rather not touch it, as it would introduce a bc break.
But123_ seems to be a valid integer too, so I need to keep that (it will simplify some regex)
2ac1ddb to0fbc94eComparexabbuh commentedApr 5, 2016
fabpot commentedApr 5, 2016
Some thoughts:
|
stof commentedApr 6, 2016
Given that this will change the way we treat the value, I'm against putting this in a patch release, as it might break existing project. This should go in master |
stof commentedApr 6, 2016
and the dumper needs to be changed, to detect that a string |
Taluu commentedApr 6, 2016
I will rebase this on |
Taluu commentedApr 9, 2016
Closing in favor of#18486 |
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.