Skip to content

Updated the DocBlocks of the Entities #527

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

Merged
merged 1 commit into from
Apr 16, 2017
Merged

Updated the DocBlocks of the Entities #527

merged 1 commit into from
Apr 16, 2017

Conversation

cafferata
Copy link
Contributor

Updated the DocBlocks of the Entities from consistency point of view.

@javiereguiluz javiereguiluz merged commit e039e60 into symfony:master Apr 16, 2017
javiereguiluz added a commit that referenced this pull request Apr 16, 2017
This PR was merged into the master branch.

Discussion
----------

Updated the DocBlocks of the Entities

Updated the DocBlocks of the Entities from consistency point of view.

Commits
-------

e039e60 Updated the DocBlocks of the Entities from consistency point of view.
@javiereguiluz
Copy link
Member

@cafferata thanks for this improvement. In 49d6ded I've removed the empty PHPdoc comments for __construct() methods because it's not a Symfony practice to do that. Thanks!

@@ -91,11 +94,17 @@ public function isLegitComment()
return !$containsInvalidCharacters;
}

/**
* @return int
Copy link
Contributor

@bocharsky-bw bocharsky-bw Apr 16, 2017

Choose a reason for hiding this comment

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

@javiereguiluz Sorry, it's probably too late, but we don't need these @return statements for getters since we already have it on properties and PhpStorm could resolve them by itself: https://github.com/cafferata/symfony-demo/blob/e039e604a31754e371811b5dba2b0eb738af07fc/src/AppBundle/Entity/Comment.php#L33

But we need them for setters which do not have type hints though.

Some discussion about it here: #394

@cafferata
Copy link
Contributor Author

@bocharsky-bw Thanks for pointing this out! Your feedback is appreciated.
@javiereguiluz What to do with this feedback?

@javiereguiluz
Copy link
Member

@cafferata yes, we should remove those statements as suggested by @bocharsky-bw.

@cafferata
Copy link
Contributor Author

@javiereguiluz I removed the @return statements based on @bocharsky-bw feedback, and push the code to the same branch after an upstream merge. Can you merge this branch again or do I need to do something else?

@javiereguiluz
Copy link
Member

@cafferata we use a custom tool for merging Symfony PRs ... and I'm afraid I cannot merge the PR again. Maybe you can create a new PR and "cherrypick" your new commit? Thanks!

@bocharsky-bw
Copy link
Contributor

bocharsky-bw commented Apr 19, 2017

I think we can just revert this merged PR, since annotations like @param \DateTime $publishedAt we don't need as well - we have type hints for arrays/objects which make this annotation pointless, and IDEs already do autocompletion based on type hints.

javiereguiluz added a commit that referenced this pull request Apr 21, 2017
…ency pov" (bocharsky-bw)

This PR was merged into the master branch.

Discussion
----------

Revert "Updated the DocBlocks of the Entities from consistency pov"

This reverts #527

The reasons were described earlier in #394

Commits
-------

88d2100 Revert "Updated the DocBlocks of the Entities from consistency point of view."
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.

3 participants