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

Documentation fixes #1

Merged
merged 1 commit into from
Apr 22, 2014
Merged

Documentation fixes #1

merged 1 commit into from
Apr 22, 2014

Conversation

BenMorel
Copy link
Contributor

Fixed doc block documentation.

@@ -31,7 +30,7 @@
final class Attributes
{
/**
* @var array<Doctrine\Common\Annotations\Annotation\Attribute>
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this one..
The Annotation Parser use @var to validate data types during the parse process.
Class[] its not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I'll revert that straight away.

@BenMorel
Copy link
Contributor Author

Any update on this PR? I can rebase it on the current master if everything is OK.

*
* @param array $data Key-value for properties to be defined in this class
* @param array $data Key-value for properties to be defined in this class.
Copy link
Member

Choose a reason for hiding this comment

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

I didn't follow what was done in your other PRs, but it seems odd to use a period after property descriptions that aren't a complete sentence. In the past, I've reserved use of periods for descriptions that happened to be two or more complete sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen that in this code base and many others in the past, so that just looked like the natural thing to do. I've indeed done that in the other PRs. That being said, I'm open to following any standard, if there's one. But we're struggling to get anywhere close, see my proposal in DDC-585!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an additional note about the dot: in my opinion, it helps identifying where the comment stops for this parameter, when it's split over several lines.

@BenMorel
Copy link
Contributor Author

Do you have plans on merging this PR?
Should I take the time to rebase it and see if there are new docs to be fixed?

@Ocramius
Copy link
Member

@BenMorel I'll merge this if you rebase - just poke me when you think I should do it :)

@@ -1,5 +1,4 @@
<?php

Copy link
Member

Choose a reason for hiding this comment

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

these changes should be reverted IMO

Copy link
Member

Choose a reason for hiding this comment

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

They really do no harm, and the PR is not really changing any code...

Copy link
Member

Choose a reason for hiding this comment

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

If we were normalizing the CS here, I would prefer adding this empty line when it is missing rather tha removing it when it is there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally don't mind, TBH I don't remember why I chose to remove it at that time. Let's just decide on either and I will follow the convention! I can start compiling some coding standards as I do that, as it's not the first point on which different people disagree.

@BenMorel
Copy link
Contributor Author

@Ocramius It's good for me!

@Ocramius
Copy link
Member

@BenMorel sorry, was AFK these days :) Merging, thanks!

Ocramius added a commit that referenced this pull request Apr 22, 2014
@Ocramius Ocramius merged commit 510afad into doctrine:master Apr 22, 2014
@Ocramius Ocramius self-assigned this Apr 22, 2014
@BenMorel
Copy link
Contributor Author

@Ocramius Thank you!

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.

5 participants