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

Property reflections include descriptions #7

Closed
wants to merge 1 commit into from
Closed

Property reflections include descriptions #7

wants to merge 1 commit into from

Conversation

janbuchar
Copy link

Sometimes it's useful to access the description of a property (e.g. automatic admin form generation). Since YetORM's EntityProperty reflection already aggregates information about all the properties, it seems to be a good candidate for this.

@@ -37,12 +40,13 @@
* @param bool $readonly
* @param string $type
Copy link
Owner

Choose a reason for hiding this comment

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

missing doctype for $description

@uestla
Copy link
Owner

uestla commented Aug 5, 2014

I added some comments (mainly typos & coding standards), however, tests would be a nice thing to have...


// parse column name
$column = $name;
if (isset($split[2], $split[3]) && $split[2] === self::PROP_COLUMN_DELIMITER) {
$column = $split[3];
if (isset($split[4])) {
$description = implode(' ', array_slice($split, 4));
Copy link
Owner

Choose a reason for hiding this comment

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

As punctilious as I am, I'd like to preserve the original whitespace (not simply glue it with one space). That would possibly mean a few more refactoring...

Copy link
Author

Choose a reason for hiding this comment

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

Yup, we would have to forget about Strings::split()... Unless we ran it twice, first to check if there's a "->" and then with an appropriate limit (which would mean using preg_split directly).

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe use a slightly more difficult (but yet single) regexp...

@janbuchar
Copy link
Author

Okay, I'll fix it according to your coding standard and update my pull request. BTW, there's a method called getReflectioin() (typo) somewhere in your reflection classes. I believe that doesn't deserve a pull request :)

@uestla
Copy link
Owner

uestla commented Aug 5, 2014

Found it! :-)

Thanks for report (fixed)

@@ -90,6 +90,7 @@ private function loadMethodProperties()

$name = lcfirst(substr($method->name, 3));
$type = $method->getAnnotation('return');
$description = $method->getAnnotation('description');
Copy link
Owner

Choose a reason for hiding this comment

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

I think that - instead of @description annotation - taking the phpDoc comment (text until first annotation) should be considered as a description...

Copy link
Owner

Choose a reason for hiding this comment

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

And of course the description would be considered at the getter method only.

@uestla uestla closed this in ef51761 Aug 5, 2014
@uestla
Copy link
Owner

uestla commented Aug 5, 2014

Implemented. Thanks for feature request.

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

Successfully merging this pull request may close these issues.

None yet

2 participants