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

Added the right parameters to the docblock. #26

Closed
wants to merge 1 commit into from

Conversation

defixje
Copy link

@defixje defixje commented Jun 29, 2020

Added the right parameters to the docblock.

@defixje defixje closed this Jun 29, 2020
@defixje defixje reopened this Jun 29, 2020
Signed-off-by: Erwin Dirks <dirks@comandi.nl>
@defixje defixje force-pushed the added-parameters-to-docblock branch from 4ca32a0 to c95eebf Compare June 29, 2020 13:04
@Xerkus Xerkus added the Invalid This doesn't seem right label Jun 29, 2020
@Xerkus
Copy link
Member

Xerkus commented Jun 29, 2020

@defixje thank you for your effort, however I am going to reject this change since Laminas coding standard dictates that documenting typehinted parameters and return values in docblock is undesirable unless additional description is needed.

That means that tags @param for some parameters as well as @return should be omitted entirely.

See https://docs.laminas.dev/laminas-coding-standard/v2/coding-style-guide/#9-commenting-and-docblocks

Code SHOULD be written so it explains itself. DocBlocks and comments SHOULD only be used if necessary. They MUST NOT start with # and MUST NOT be empty. They SHOULD NOT be used for already typehinted arguments, except arrays.

@Xerkus Xerkus closed this Jun 29, 2020
@Xerkus
Copy link
Member

Xerkus commented Jun 29, 2020

Sorry, our coding standard documentation is not clear about this. I opened new issue to clarify the rule laminas/laminas-coding-standard#33

@defixje
Copy link
Author

defixje commented Jun 29, 2020

Ah ok, i get it. No problem. Thanks for the explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants