Skip to content

Type hiearchy for property types and enums#256

Merged
Ocramius merged 3 commits intodoctrine:masterfrom
Majkl578:type
Apr 3, 2019
Merged

Type hiearchy for property types and enums#256
Ocramius merged 3 commits intodoctrine:masterfrom
Majkl578:type

Conversation

@Majkl578
Copy link
Copy Markdown
Contributor

@Majkl578 Majkl578 commented Mar 14, 2019

This PR introduces a type hiearchy for annotation properties - types and enums.

Summary:

  • introduces rather simple type hiearchy with validation and describe support;
  • contains support for constant scalar types;
  • describe() uses phpDoc-friendly format (plus intersections and constant values);
  • validation methods return booleans instead of throwing;
  • property metadata now use type objects instead of arrays for type and enum information;
  • moves validation decisions out of DocParser into type objects themselves;
  • moves type info formatting out of DocParser into type objects themselves.

I didn't put much effort into DocParser changes as it's eventually going to disappear.

Future scope:

  • merge type and enum into one,
  • rework parser for @var to properly support unions and unqualified names.

Coverage in Type namespace is 100%. 😃

Comment thread lib/Doctrine/Annotations/DocParser.php
jwage
jwage previously approved these changes Mar 29, 2019
@Majkl578 Majkl578 added this to the v2.0.0 milestone Mar 29, 2019
Copy link
Copy Markdown
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

I like this overall, good job! Couple comments that need to be addressed

Comment thread lib/Doctrine/Annotations/DocParser.php Outdated
Comment thread lib/Doctrine/Annotations/DocParser.php
Comment thread lib/Doctrine/Annotations/DocParser.php
Comment thread lib/Doctrine/Annotations/Type/MixedType.php Outdated
Comment thread lib/Doctrine/Annotations/Type/ArrayType.php
Comment thread lib/Doctrine/Annotations/Type/CompositeType.php
Comment thread tests/Doctrine/Tests/Annotations/Type/ArrayTypeTest.php
Comment thread tests/Doctrine/Tests/Annotations/Type/ArrayTypeTest.php
Comment thread lib/Doctrine/Annotations/Type/Constant/ConstantFloatType.php Outdated
Copy link
Copy Markdown
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.

Overall good improvement, but some confusion between types and values (mostly a question of naming and documentation).

If we think of the definition of "type" as a "set" of possible values, then it makes sense to have IntegerType as a set of all possible int passing Type#validate(), but for ConstantType (a type that is represented by only one possible value, as far as I can see), having ConstantType#getValue() is confusing to me, and I couldn't really find usages.

The reason is that a type type doesn't "have" a value: it is logically always a set, sometimes with only ever one item, or no possible type at all, such as BottomType (impossible).

*/
interface Type
{
public function describe() : string;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A description of the intent of this method is needed. What is the "description" you will get from this? Human-readable? Machine-friendly? An identifier? The type FQN?

Comment thread lib/Doctrine/Annotations/Type/ConstantType.php Outdated
@Majkl578
Copy link
Copy Markdown
Contributor Author

Majkl578 commented Apr 3, 2019

for ConstantType (a type that is represented by only one possible value, as far as I can see), having ConstantType#getValue() is confusing to me, and I couldn't really find usages.

Technically getValue()is not currently needed so can as well be dropped.

@Ocramius
Copy link
Copy Markdown
Member

Ocramius commented Apr 3, 2019

Technically getValue() is not currently needed so can as well be dropped.

That's what I'm mostly sniping at

@Majkl578
Copy link
Copy Markdown
Contributor Author

Majkl578 commented Apr 3, 2019

That's what I'm mostly sniping at

Dropped. 🤞

@Ocramius Ocramius self-assigned this Apr 3, 2019
@Ocramius Ocramius merged commit ae55c78 into doctrine:master Apr 3, 2019
@Ocramius
Copy link
Copy Markdown
Member

Ocramius commented Apr 3, 2019

Nice work! 👍

@Majkl578 Majkl578 deleted the type branch April 3, 2019 17:00
@Majkl578 Majkl578 mentioned this pull request Apr 3, 2019
27 tasks
@alcaeus alcaeus mentioned this pull request May 8, 2020
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.

5 participants