8000 [Yaml] deprecate comma separators in floats by xabbuh · Pull Request #18785 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Yaml] deprecate comma separators in floats #18785

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 24, 2016
Merged

[Yaml] deprecate comma separators in floats #18785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/Yaml/Inline.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,7 @@ private static function evaluateScalar($scalar, $flags, $references = array())
case 0 === strpos($scalar, '!!binary '):
return self::evaluateBinaryScalar(substr($scalar, 9));
case preg_match('/^(-|\+)?[0-9][0-9,]*(\.[0-9_]+)?$/', $scalar):
@trigger_error('Using the comma as a group separator for floats is deprecated since version 3.2 and will be removed in 4.0.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this regexp right, the deprecation error will be triggered for example for numbers like 123.45 but it shouldn't. We'd need to test explicitly the presence of the comma.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the order of regexps should be changed, to match the standard regex first to handle the case of values matching the spec
This will require to duplicate the line return (float) str_replace(array(',', '_'), '', $scalar); instead of using a fallthrough, but this is not an issue.

Copy link
Member Author
@xabbuh xabbuh May 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cases are already handled further up where we have the is_numeric() check (and we have tests covering that this is working).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @javiereguiluz said, this deprecation will be triggered with 123.45, which is not correct. IMO, the * should be replaced with a + to ensure that a , is indeed used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the + won't be enough ; we need to test that [0-9]+,[0-9]*(?:\.[0-9_]+)? matches this I think (but I'm not too sure too)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taluu but it's true what @xabbuh said: this is handled in the code above this case: and there are tests about this and they pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it should be triggered for 123.45_67 though (I think), as there are no cases for that above this case, and that is valid though. And I think a _ may have been forgotten in the part before the . actually.

IMO, we should do a strpos test to test that a , is indeed in the match, and then only trigger the deprecation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taluu You are right. I opened #18858 which should fix this issue.

And I think a _ may have been forgotten in the part before the . actually.

I think it's okay the way it is. We only introduced the _ for 3.2 and I think people should not start mixing _ and , then. If they want to use _, they should just move away from , as well.

case preg_match('/^(-|\+)?[0-9][0-9_]*(\.[0-9_]+)?$/', $scalar):
return (float) str_replace(array(',', '_'), '', $scalar);
case preg_match(self::getTimestampRegex(), $scalar):
Expand Down
45 changes: 37 additions & 8 deletions src/Symfony/Component/Yaml/Tests/Fixtures/YtsSpecificationExamples.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,37 +509,53 @@ test: Integers
spec: 2.19
yaml: |
canonical: 12345
decimal: +12,345
octal: 014
hexadecimal: 0xC
php: |
array(
'canonical' => 12345,
'decimal' => 12345.0,
'octal' => 014,
'hexadecimal' => 0xC
)
---
test: Decimal Integer
deprecated: true
spec: 2.19
yaml: |
decimal: +12,345
php: |
array(
'decimal' => 12345.0,
)
---
# FIX: spec shows parens around -inf and NaN
test: Floating point
spec: 2.20
yaml: |
canonical: 1.23015e+3
exponential: 12.3015e+02
fixed: 1,230.15
negative infinity: -.inf
not a number: .NaN
float as whole number: !!float 1
php: |
array(
'canonical' => 1230.15,
'exponential' => 1230.15,
'fixed' => 1230.15,
'negative infinity' => log(0),
'not a number' => -log(0),
'float as whole number' => (float) 1
)
---
test: Fixed Floating point
deprecated: true
spec: 2.20
yaml: |
fixed: 1,230.15
php: |
array(
'fixed' => 1230.15,
)
---
test: Miscellaneous
spec: 2.21
yaml: |
Expand Down Expand Up @@ -1532,29 +1548,42 @@ php: |
test: Integer
yaml: |
canonical: 12345
decimal: +12,345
octal: 014
hexadecimal: 0xC
php: |
array(
'canonical' => 12345,
'decimal' => 12345.0,
'octal' => 12,
'hexadecimal' => 12
)
---
test: Decimal
deprecated: true
yaml: |
decimal: +12,345
php: |
array(
'decimal' => 12345.0,
)
---
test: Fixed Float
deprecated: true
yaml: |
fixed: 1,230.15
php: |
array(
'fixed' => 1230.15,
)
test: Float
yaml: |
canonical: 1.23015e+3
exponential: 12.3015e+02
fixed: 1,230.15
negative infinity: -.inf
not a number: .NaN
php: |
array(
'canonical' => 1230.15,
'exponential' => 1230.15,
'fixed' => 1230.15,
'negative infinity' => log(0),
'not a number' => -log(0)
)
Expand Down
44 changes: 40 additions & 4 deletions src/Symfony/Component/Yaml/Tests/Fixtures/YtsTypeTransfers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,37 @@ brief: >
yaml: |
zero: 0
simple: 12
one-thousand: 1,000
negative one-thousand: -1,000
php: |
array(
'zero' => 0,
'simple' => 12,
)
---
test: Positive Big Integer
deprecated: true
dump_skip: true
brief: >
An integer is a series of numbers, optionally
starting with a positive or negative sign. Integers
may also contain commas for readability.
yaml: |
one-thousand: 1,000
php: |
array(
'one-thousand' => 1000.0,
)
---
test: Negative Big Integer
deprecated: true
dump_skip: true
brief: >
An integer is a series of numbers, optionally
starting with a positive or negative sign. Integers
may also contain commas for readability.
yaml: |
negative one-thousand: -1,000
php: |
array(
'negative one-thousand' => -1000.0
)
---
Expand All @@ -208,15 +232,27 @@ brief: >
positive and negative infinity and "not a number."
yaml: |
a simple float: 2.00
larger float: 1,000.09
scientific notation: 1.00009e+3
php: |
array(
'a simple float' => 2.0,
'larger float' => 1000.09,
'scientific notation' => 1000.09
)
---
test: Larger Float
dump_skip: true
deprecated: true
brief: >
Floats are represented by numbers with decimals,
allowing for scientific notation, as well as
positive and negative infinity and "not a number."
yaml: |
larger float: 1,000.09
php: |
array(
'larger float' => 1000.09,
)
---
test: Time
todo: true
brief: >
Expand Down
25 changes: 23 additions & 2 deletions src/Symfony/Component/Yaml/Tests/ParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,30 @@ protected function tearDown()
/**
* @dataProvider getDataFormSpecifications
*/
public function testSpecifications($file, $expected, $yaml, $comment)
public function testSpecifications($file, $expected, $yaml, $comment, $deprecated)
{
$deprecations = array();

if ($deprecated) {
set_error_handler(function ($type, $msg) use (&$deprecations) {
if (E_USER_DEPRECATED !== $type) {
restore_error_handler();

return call_user_func_array('PHPUnit_Util_ErrorHandler::handleError', func_get_args());
}

$deprecations[] = $msg;
});
}

$this->assertEquals($expected, var_export($this->parser->parse($yaml), true), $comment);

if ($deprecated) {
restore_error_handler();

$this->assertCount(1, $deprecations);
$this->assertContains('Using the comma as a group separator for floats is deprecated since version 3.2 and will be removed in 4.0.', $deprecations[0]);
}
}

public function getDataFormSpecifications()
Expand All @@ -58,7 +79,7 @@ public function getDataFormSpecifications()
} else {
eval('$expected = '.trim($test['php']).';');

$tests[] = array($file, var_export($expected, true), $test['yaml'], $test['test']);
$tests[] = array($file, var_export($expected, true), $test['yaml'], $test['test'], isset($test['deprecated']) ? $test['deprecated'] : false);
}
}
}
Expand Down
0