-
-
Notifications
You must be signed in to change notification settings - Fork 42
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 parsing and checking functions docblocks #302
Conversation
Codecov Report
@@ Coverage Diff @@
## main #302 +/- ##
============================================
+ Coverage 95.87% 96.06% +0.18%
- Complexity 397 399 +2
============================================
Files 59 59
Lines 1091 1092 +1
============================================
+ Hits 1046 1049 +3
+ Misses 45 43 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice! Left a minor, non blocking comment
@@ -133,6 +127,12 @@ public function enterNode(Node $node): void | |||
); | |||
} | |||
|
|||
if (null !== $this->classDescriptionBuilder && null !== $node->getDocComment()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the null check on $this->classDescriptionBuilder
? I find it a bit confusing, as we are already using $this->classDescriptionBuilder
a few lines above without any check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have got different errors without this check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I got what you mean but just out of curiosity: doesn't the check on line 28 prevent from having a null in $this->classDescriptionBuilder
here?
And just to be super clear: all good from me, feel free to merge it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check on line 28 is inside an if that finish before the new check, this check is more generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little bit confusing, if tou want to split this method this could be a great refactoring... but if you prefer we can work on it on another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split the if @fain182 ? What is confusing in this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This should fix this issue #301
Is it a BC?