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

Fill in class and property documentation based on Database Comments #226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelarnauts
Copy link

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #131,

Depends on yiisoft/yii2#13162 for table comments.

I'm not sure if there has to be an option to enable this. This is just for comments, and doesn't have an impact on functionality or code.

*
<?php foreach ($tableSchema->columns as $column): ?>
* @property <?= "{$column->phpType} \${$column->name}\n" ?>
* @property <?= "{$column->phpType} \${$column->name}" . ($column->comment ? " " . strtr($column->comment, ["\n" => "\n * "]) : '') . "\n" ?>
Copy link
Member

Choose a reason for hiding this comment

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

Can't be used like that since there's already an option in Gii Model generator "Generate Labels from DB Comments".

Copy link
Member

Choose a reason for hiding this comment

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

nope, labels are not generated from table comments, just column comments. these are not conflicting.

Copy link
Member

Choose a reason for hiding this comment

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

This line is about column comments.

Copy link
Member

Choose a reason for hiding this comment

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

right, but now I'm confused, how are column comments handled if not in this line!?

Copy link
Author

Choose a reason for hiding this comment

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

Why can't we use db comments for form labels and also property documentation at the same time? It does make sense, no?

Copy link
Member

Choose a reason for hiding this comment

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

okay, now I see. This line should respect the $generateLabelsFromComments and only add the comments here if they are not used as labels.

Copy link
Member

Choose a reason for hiding this comment

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

actually, you are right, @michaelarnauts we could still add them here, better have the label, than no documentation or description at all.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about it... it makes sense.

*
<?php foreach ($tableSchema->columns as $column): ?>
* @property <?= "{$column->phpType} \${$column->name}\n" ?>
* @property <?= "{$column->phpType} \${$column->name}" . ($column->comment ? " " . strtr($column->comment, ["\n" => "\n * "]) : '') . "\n" ?>
Copy link
Member

Choose a reason for hiding this comment

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

right, but now I'm confused, how are column comments handled if not in this line!?

*
<?php foreach ($tableSchema->columns as $column): ?>
* @property <?= "{$column->phpType} \${$column->name}\n" ?>
* @property <?= "{$column->phpType} \${$column->name}" . ($column->comment ? " " . strtr($column->comment, ["\n" => "\n * "]) : '') . "\n" ?>
Copy link
Member

Choose a reason for hiding this comment

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

okay, now I see. This line should respect the $generateLabelsFromComments and only add the comments here if they are not used as labels.

@michaelarnauts
Copy link
Author

What has kept this pull request from being merged? (It seems that is has gotten conflicts in the meantime). Can this be accepted when the conflicts are resolved?

@michaelarnauts
Copy link
Author

To recap, this doesn't change any code or changes the model in any way. It just makes sure that db comments are added to the phpdoc, so any IDE can use this to give additional information about the property or table object.

'type' => $type,
'name' => $column->name,
'description' => $column->comment,
];
Copy link
Member

Choose a reason for hiding this comment

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

seems your last commit changed the diff to something wrong. This change looks unrelated to the changes below...

Copy link
Member

Choose a reason for hiding this comment

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

well, at least comment and description do not match.

Copy link
Author

Choose a reason for hiding this comment

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

I also need the comments, so it cant be just key => value. The name is just for completeness.

Copy link
Member

Choose a reason for hiding this comment

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

sure, but you are creating the key description here, which you are using comment in the code below.

Copy link
Author

Choose a reason for hiding this comment

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

Ow, right. Sorry, will fix that.

@cebe cebe added this to the 2.0.6 milestone Feb 2, 2017
@cebe cebe added the type:enhancement Enhancement label Feb 2, 2017
@samdark samdark modified the milestones: 2.0.x, 2.0.6 Feb 2, 2017
@cebe cebe self-assigned this Feb 2, 2017
@cebe cebe modified the milestones: 2.0.6, 2.0.x Feb 2, 2017
@cebe cebe closed this in a8a3cda Feb 2, 2017
@cebe
Copy link
Member

cebe commented Feb 2, 2017

I have merged the property comment part, thank you! Keeping this open for the class comment when it is supported in the framework.

@cebe cebe reopened this Feb 2, 2017
@cebe cebe modified the milestones: 2.0.x, 2.0.6 Feb 2, 2017
@samdark samdark removed this from the 2.0.x milestone Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants