Skip to content
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

allow empty strings and numbers as metadata type parameters #1019

Merged
merged 2 commits into from Dec 12, 2018

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Dec 9, 2018

Q A
Bug fix? yes
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets schmittjoh/JMSSerializerBundle#706
License MIT

@Majkl578 do you mind having a look at this?

@@ -72,14 +92,6 @@ public function validTypesProvider(): iterable
'Foo<"asdf asdf">',
$type('Foo', ['asdf asdf']),
];
yield [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed this as it will not allow '' (empty strings syntax) and was not available in the 1.x parser

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I can guess, what you were trying to do here was handled by YAML

@goetas goetas changed the title allow empty strings and numbers as parameters allow empty strings and numbers as metadata type parameters Dec 9, 2018
@goetas goetas merged commit 0e17bb6 into master Dec 12, 2018
@goetas goetas deleted the extra-type-tests branch December 12, 2018 07:59
@@ -2,22 +2,28 @@

%token parenthesis_ <
%token _parenthesis >
%token empty_string ""|''
%token number (\+|\-)?(0|[1-9]\d*)(\.\d+)?
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't allow floats in the following form: .123
Should be written as something similar:

[+-]?(?:(?:0|[1-9]\d*(?:\.0*[1-9]\d*)?)|\.0*\d*[1-9])

https://regex101.com/r/Nadose/1

Also this doesn't handle exponential floats.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true. My goal was to solve the reported bug. A more complete implementation is welcome

@@ -2,22 +2,28 @@

%token parenthesis_ <
%token _parenthesis >
%token empty_string ""|''
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird, I think this should be handled as part of strings themselves, meaning that they will no longer be a single token but a node instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was the only way I found to make it work with empty strings.
Better solutions are welcome

Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the inner token optional in simple_type (<quoted_string>? and <apostrophed_string>?), it will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried right now,

1) JMS\Serializer\Tests\Serializer\Type\ParserTest::testParse with data set #7 ('Foo<'a',''>', array('Foo', array('a', '')))
Error: Call to a member function getValueToken() on null

/home/goetas/projects/serializer/src/Type/TypeVisitor.php:36
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:22
/home/goetas/projects/serializer/vendor/hoa/compiler/Llk/TreeNode.php:333
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:69
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:71
/home/goetas/projects/serializer/src/Type/TypeVisitor.php:24
/home/goetas/projects/serializer/src/Type/Parser.php:30
/home/goetas/projects/serializer/tests/Serializer/Type/ParserTest.php:29

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, you made the token optional (nullable), so the visitor needs to be updated accordingly. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants