Skip to content

Conversation

@rlweb
Copy link

@rlweb rlweb commented Feb 17, 2017

As said, added square brackets to complete support for JSON.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Code looks good, but we need to improve the testing of nested values, optional additional values.

This should be done both in the parser tests, and in the annotation reader afterwards, in my opinion.

@guilhermeblanco any opinion on supporting this?

array('array', '{@AnnotationExtendsAnnotationTargetAll}'),
array('array', '{@AnnotationExtendsAnnotationTargetAll,@AnnotationExtendsAnnotationTargetAll}'),
array('array', '[@AnnotationExtendsAnnotationTargetAll]'),
array('array', '[@AnnotationExtendsAnnotationTargetAll,@AnnotationExtendsAnnotationTargetAll]'),
Copy link
Member

Choose a reason for hiding this comment

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

Would need a few tests around parsing array elements, their order, and different types of sub-elements

Copy link
Member

Choose a reason for hiding this comment

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

Also nested arrays

const T_TRUE = 110;
const T_NULL = 111;
const T_COLON = 112;
const T_OPEN_SQR_BRACKETS = 113;
Copy link
Member

Choose a reason for hiding this comment

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

SQUARE

const T_NULL = 111;
const T_COLON = 112;
const T_OPEN_SQR_BRACKETS = 113;
const T_CLOSE_SQR_BRACKETS = 114;
Copy link
Member

Choose a reason for hiding this comment

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

SQUARE

@Majkl578
Copy link
Contributor

This is something that should be relatively easy with the new parser in Annotations 2.0. Milestoning, since there is no relevant issue, but it will require different implementation.

@Majkl578 Majkl578 added this to the v2.0.0 milestone Oct 16, 2018
@rlweb rlweb closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants