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

Do not remove "class foo" comments. #122

Closed
wants to merge 1 commit into from

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel requested a review from a team as a code owner April 26, 2019 02:12
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.

Disagree here: "class foo" in comments is easily replaced by @see FQCN instead, which is more valuable.

@greg0ire
Copy link
Member

@Ocramius the issue at hand is "Initializes this instance with the specified document manager, unit of work and\nclass metadata." being unduly truncated, "Initializes this instance with the specified document manager, unit of work and\n@see foo" would raise some eyebrows :P . IMO the right fix would be to remove it iff it the line before is /**, but that's probably not possible, the best thing after that probably being @carusogabriel 's solution.

@@ -10,7 +10,8 @@
use Doctrine\Sniffs\Spacing\ControlStructureSniff;

/**
* Description
* Description of the
* class example.
Copy link
Member

@greg0ire greg0ire Apr 26, 2019

Choose a reason for hiding this comment

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

The build fails, you are probably supposed to add this in the output class too.

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Issue is easily worked around by splitting the sentence differently. I think few false positives with this workaround are better than missing some some useless comments.

@malarzm
Copy link
Member

malarzm commented Apr 26, 2019

Issue is easily worked around by splitting the sentence differently.

Come on, this is a hack for a broken rule

@ostrolucky
Copy link
Member

ostrolucky commented Apr 26, 2019

I wouldn't say it's broken, just imperfect. And let's not make "hack" word even more meaningless than it is. Hitting enter at different part of sentence is hardly hacking. Regex could be improved by taking preceding content into consideration. Just making it case sensitive is lazy solution impacting false negatives too much IMHO.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Depending on the context of the comment, I agree with @Ocramius to add @see. But something like a broken build because of class foo in a comment is an unexpected behavior for a user, which is why I'm for this change. "class" won't always be a reference to another class in a comment:

/**
 * Lorem ipsum dolor sit amet. That is why we like to have our
 * class final
 */

@ostrolucky
Copy link
Member

Regex could be improved by taking preceding content into consideration

@carusogabriel carusogabriel deleted the bugfix/class-case-sensitive branch September 22, 2019 17:37
@carusogabriel carusogabriel added invalid and removed bug labels Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants