Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Should be a new line between description and @param.
Hardly seems worth opening an issue for, but should be sorted one way or another.
Proposed resolution
Add the line
Remaining tasks
Review and commit.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-18-22.txt | 2.34 KB | pk188 |
#22 | 2842949-22.patch | 36.66 KB | harsha012 |
#18 | interdiff-10-18.txt | 2.38 KB | gaurav.kapoor |
#18 | drupal-comment-coding-standard-2842949-18-d8.patch | 34.76 KB | gaurav.kapoor |
#12 | drupal-comment-coding-standard-2842949-10-d8.patch | 36.12 KB | rigoucr |
Comments
Comment #2
cilefen CreditAttribution: cilefen as a volunteer commentedI think we've been scoping these like this. I count 27 cases in core/modules alone.
Comment #4
Ketan Harit CreditAttribution: Ketan Harit commentedComment #5
arunkumarkHi,
I have re-rolled patch for latest Drupal 8.4.x version.
Comment #6
oo0shiny CreditAttribution: oo0shiny at Genuine commentedTested this locally on an 8.4.x install. Patch applied successfully and the changes showed up as they should. Moving this to RTBC!
Comment #7
dawehner@arunkumark
On top of that we need to add the new rule into our phpcs.xml.dist file, so we can get automatic checking of it.
Comment #8
cilefen CreditAttribution: cilefen as a volunteer commentedPlus, see comment #2, where I found more than one.
Comment #9
rigoucr CreditAttribution: rigoucr as a volunteer and at Manatí commentedI removed the phpcs rule from the blacklist and fixed the errors
Comment #11
rigoucr CreditAttribution: rigoucr as a volunteer and at Manatí commentedAll the tests passed
Comment #12
rigoucr CreditAttribution: rigoucr as a volunteer and at Manatí commentedRe-rolled patch
Comment #13
kporras07 CreditAttribution: kporras07 at Manatí commentedHi,
I just tested this and all coding standard passes after removing the rule from the blacklist with the patch in #12.
Marking as RTBC
Comment #14
kporras07 CreditAttribution: kporras07 at Manatí commentedoops! Now I marked it as RTBC
Comment #15
Gábor HojtsyIt is important to review generated patches especially. Looking through at least found these problems and a couple questions:
This change does not seem to be right.
Also does not seem to be the right change.
Should this newline change apply to @todo items in a list like this?
Also incorrect.
Hm, why are we removing this?
Comment #16
dawehner@Gábor Hojtsy
I'm confused by your review ... So
This means we need a @code @endcode statement around this line, I guess?
What this patch is doing is to no longer exclude the "SpcaingBeforeTags" rule, so removing this line is exactly expected.
Comment #17
Gábor Hojtsy@dawehner: you are right on point 2, I did not realize that is *phpdoc* tags, my bad, sorry.
For the others, I don't necessarily know what the fix would be, but when a phpdoc tag is on the first character of a line, it could also be part of a sentence as shown in the examples I found. I am not sure putting them in code tags is necessarily better, you would need to put the other one as well in the example you cited. But the proposed change for those lines does seem like incorrect.
Comment #18
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedFixed according to suggestions in 15,16 and 17. Still not sure about this change.
Comment #20
Gábor HojtsyI don't think removing the newlines would work as the fail showed, since those lines do not match the coding standards then :) Yet adding the newlines would break the code comments.
Comment #21
xjmThe fail is not about coding standards, because the automated failures for the coding standards are not happening yet. You can see the actual fail from the "view results" link: https://www.drupal.org/pift-ci-job/695012 That's an actual test failure in
LazyContextRepositoryTest
.The coding style fails are further down on the results:
In SqlContentEntityStorage and EntityStorageInterface; they are false fails that we need to skip somehow, reorderm or address in coder.
In LogMessageParserInterface the comment is just malformatted entirely. @see should be the last section in the docblock, not inline in another section. If we really absolurely need to link it inline in the exact spot, link tags can be used inline instead like:
See @link whatever @endlink.
The comment should probably be rewritten for LogMessageParserInterface; I'd open a separate blocking issue for that.
Comment #22
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedfixing the test and also the coding standard.
Comment #23
pk188 CreditAttribution: pk188 at OpenSense Labs commentedInterdiff #18 and #22. Not sure about some changes of #22.
Comment #24
xjmThanks @pk188 for the interdiff.
Unfortunately none of the changes from #22 are complete or correct, so I'm removing the issue credit for now for @harsha012. Let's make sure to only change things in the way that they should be changed, rather than rewriting them in an incorrect way to make the rule pass.
We'll need to fix this to be
@link
as well.I am fairly certain this change is not correct as the
@
is actually meaningful here. Seecore/themes/seven/templates/block--local-actions-block.html.twig
which has:For this line, we could enclose them in single quotes, but I actually think the coder rule has too many false positives. Maybe the rule should check against a list of defined docblock tags.
These
@see
should be moved to the end of their docblocks instead, or should use@link
. Simply adding a blank line and leaving the list indentation will cause it to be malformed.As @sugaroverflow and I discussed a bit ago, this code is a third-party library that we've pulled into core, so it should probably be skipped. #2887052: Ignore Diff component files in phpcs coding standards is the related issue to skip it. So once that lands, we can remove these hunks from the patch.
This is just splitting up a paragraph into two paragraphs and is not correct. I'm starting to think we need to improve the rule somehow to only check for existing docblock tags rather than anything with an
@
sign.This change is incorrect and should be removed.
Here as well, removing the
@
is incorrect, because it is actually meaningful.The @todo should be moved to the bottom of the docblock (with more explanation of "this" and a dedicated issue for it... also, clearly, this did not get fixed in Drupal 8).
Hmm, this change is not really correct either.
This should be wrapped in a
@code
/@endcode
and indented properly instead.So, before we proceed with this issue, we should maybe open an issue to discuss whether to make this coder rule more focused so there are fewer false positives. Thanks!
Comment #25
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented@xjm , is this issue queue is postponed till we get the proper solution related to the coding standard issue for doc block.
Comment #31
jungleComment #34
quietone CreditAttribution: quietone as a volunteer commentedPretty sure all the coding standards issues are Tasks not Bugs.