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

[Contributing] [Standards] Added note about phpdoc_separation #5446

Closed
wants to merge 1 commit into from

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Jun 26, 2015

Q A
Doc fix? no
New docs? no
Applies to 2.8+
Fixed tickets

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | no
| Applies to    | 2.8+
| Fixed tickets |
@@ -172,6 +172,10 @@ Documentation

* Add PHPDoc blocks for all classes, methods, and functions;

* Group annotations together so that annotations of the same type immediately
Copy link
Member

Choose a reason for hiding this comment

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

I think this proposal makes sense ... but in some cases it can be difficult to decide the annotation type. @ORM and @Assert obviously define their own groups, but what about @Cache, @Method, @Route, @Security Are these four annotations of the same type?

Copy link
Member

Choose a reason for hiding this comment

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

this rule is in the "Documentation" section. This means it's focused on the PHPdoc annotations

Copy link
Member

Choose a reason for hiding this comment

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

I see. But my question remains ... what is an "annotation type"?

Copy link
Member

Choose a reason for hiding this comment

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

@param, @return, @throws, @author are all different types. Just different annotations

Copy link
Member

Choose a reason for hiding this comment

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

OK. I finally understood it. So maybe we could say something like "group annotations by name"?

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 just copied the text from php-cs-fixer:

phpdoc_separation [symfony]
Annotations in phpdocs should be grouped together so that annotations of the same type immediately follow each other, and annotations of a different type are separated by a single blank line.

@phansys
Copy link
Contributor Author

phansys commented Jun 26, 2015

My idea is to document all the rules tagged as [symfony] in php-cs-fixer within separated PRs and later add a note about the possibility of running the tool with --dry-run option in order to check if all standards are respected.
What do you think?

@wouterj
Copy link
Member

wouterj commented Jun 26, 2015

👍 for merging this PR!

@javiereguiluz
Copy link
Member

I agree with @wouterj: this PR looks mergeable. Please forget my comment above.

xabbuh added a commit that referenced this pull request Dec 17, 2015
…tion (phansys)

This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #5446).

Discussion
----------

[Contributing] [Standards] Added note about phpdoc_separation

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | no
| Applies to    | 2.8+
| Fixed tickets |

Commits
-------

fd7c335 [Contributing] [Standards] Added note about phpdoc_separation.
@xabbuh
Copy link
Member

xabbuh commented Dec 17, 2015

Thank you very much for this nice improvement @phansys and sorry for the delay. I merged this into the 2.3 branch. That's why the PR is displayed as closed instead of merged.

@xabbuh xabbuh closed this Dec 17, 2015
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