Skip to content

Conversation

xalopp
Copy link
Contributor

@xalopp xalopp commented Oct 19, 2018

No description provided.

@xalopp xalopp changed the title fix no multiline comments after declarations fix no multiline comments after declarations, fixes #96 Oct 19, 2018
@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #104 into master will increase coverage by 1.52%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master   #104      +/-   ##
==========================================
+ Coverage     98.47%   100%   +1.52%     
  Complexity      124    124              
==========================================
  Files             6      6              
  Lines           523    520       -3     
==========================================
+ Hits            515    520       +5     
+ Misses            8      0       -8
Impacted Files Coverage Δ Complexity Δ
MO4/Sniffs/Commenting/PropertyCommentSniff.php 100% <100%> (+8.42%) 22 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1467198...444f498. Read the comment docs.

Copy link
Contributor

@mmoll mmoll left a comment

Choose a reason for hiding this comment

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

💯

@mmoll mmoll merged commit d1d9864 into mayflower:master Oct 22, 2018
@@ -42,6 +42,8 @@ class X
/** @var int $bla */
$bla = 1;
/** @var int $bla */

return ($bla === $foo); /* yeah! */
Copy link
Member

Choose a reason for hiding this comment

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

why /* yeah! */? it reads awkward if in the same changeset with /* invalid doc block y is null by default */.
As the later suggests any comment after declarations/code is unwanted.

P.S.
I see, only after declarations. Maybe state this in the faulty comment above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calbrecht absolutely right. /* yeah! */ is misleading. I'll replace it with /* allowed single line comment */. That should settle this.

Thank for this insightful comment 👍

multiline
comment */

private $y = null; /** invalid doc block y is null by default */
private $q = null; /* invalid doc block y is null by default */
Copy link
Member

Choose a reason for hiding this comment

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

/* invalid doc block after declaration */ -- please :), same above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. This should be /* valid single line comment */

@xalopp xalopp mentioned this pull request Oct 22, 2018
Ma27 added a commit that referenced this pull request Oct 22, 2018
Initially discussed in #104 (review)
we want to have comments that actually tell the developer *why*
the comment itself is invalid or misplaced.

/cc @calbrecht
mmoll pushed a commit that referenced this pull request Oct 23, 2018
Initially discussed in #104 (review)
we want to have comments that actually tell the developer *why*
the comment itself is invalid or misplaced.

/cc @calbrecht
mmoll pushed a commit to mmoll/mo4-coding-standard that referenced this pull request Oct 23, 2018
Initially discussed in mayflower#104 (review)
we want to have comments that actually tell the developer *why*
the comment itself is invalid or misplaced.

/cc @calbrecht

(cherry picked from commit 4806232)
xalopp pushed a commit that referenced this pull request Oct 24, 2018
Initially discussed in #104 (review)
we want to have comments that actually tell the developer *why*
the comment itself is invalid or misplaced.

/cc @calbrecht

(cherry picked from commit 4806232)
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