Skip to content

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Mar 2, 2018

There is also counerpart sniff: DisallowOneLinePropertyDocComment.

I personally prefer the one-line variant, so starting with that one here.

ORM currently has lots of multilines for single undocumented @var.

@Majkl578 Majkl578 added this to the 4.0.0 milestone Mar 2, 2018
@carusogabriel
Copy link
Contributor

carusogabriel commented Mar 3, 2018

For now I'm 👎, but here's why: ORM/tests current has 100+ missing @var, and I think we should focus in add them, and maybe letter simplify :)

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM!

@carusogabriel since this will be in a new major version, ORM can either not upgrade or disable the new sniff until it’s appropriate to enable it and fix issues. That’s what we do in ODM to cope with the sheer number of checkstyles.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

This sniff doesn't necessarily collide with having to add docblocks, so let's 🚢 :)

@Ocramius
Copy link
Member

Ocramius commented Mar 3, 2018

@Majkl578 is this still WIP?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Mar 3, 2018

If there are no objections, we can land this. I marked it as WIP & RFC since I expected discussion for require vs. disallow one-liners. 👍

@Ocramius Ocramius self-assigned this Mar 3, 2018
@Ocramius Ocramius merged commit 0469c18 into master Mar 3, 2018
@Ocramius Ocramius deleted the sniff/RequireOneLinePropertyDocComment branch March 3, 2018 23:49
@deeky666
Copy link
Member

deeky666 commented Apr 5, 2018

Isn't this a rule that should be applied to all doc comments? Why only on properties and not constants, methods, classes etc? Just wondering...

@Ocramius
Copy link
Member

Ocramius commented Apr 6, 2018

@deeky666 yes, but let's please RFC any additions please 😁

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.

5 participants