Problem/Motivation

The changes required for 'Drupal.Commenting.DocComment.ShortSingleLine' are too large and need to be split into child issues.

Proposed resolution

Fix class property comments for sniff 'Drupal.Commenting.DocComment.ShortSingleLine'.

Remaining tasks

Patch
Review - Instructions for adding the sniff and running the test are in the Issue Summary of #3123060: Enable 'Drupal.Commenting.DocComment.ShortSingleLine' coding standard
Commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3268829-13-9.5.x.patch35.63 KBquietone
#13 interdiff-8-13-9.5.x.txt630 bytesquietone
#12 interdiff-3-8.txt2.33 KBquietone
#12 diff-3-8-v2.txt11.54 KBquietone
#8 3268829-8.patch35.64 KBquietone
#8 diff-3-8.txt5 KBquietone
#3 3268829-3.patch34.24 KBquietone
#3 interdiff-2-3.txt994 bytesquietone
#2 3268829-2.patch33.27 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 3268829-2.patch33.27 KB
quietone’s picture

quietone commented
StatusFileSize
new interdiff-2-3.txt994 bytes
new 3268829-3.patch34.24 KB

Move property changes from #3268833: Fix method comments in tests for Drupal.Commenting.DocComment.ShortSingleLine to here.

tr’s picture

tr commented

Somehow testing never got triggered on the patch in #3, so I triggered it manually.

quietone’s picture

quietone commented
Issue tags: +beta blocker
quietone’s picture

quietone commented
Issue tags: -beta blocker +beta target

Wrong tag.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Ok, went through all the results and found a few missing changes:



FILE: /home/localcopy/drupal-core-10-0-x/web/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 10 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------------
   76 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
      |       | (Drupal.Commenting.DocComment.ShortSingleLine)
-------------------------------------------------------------------------------------------------------------------------------------------------------------

-------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /home/localcopy/drupal-core-10-0-x/web/core/lib/Drupal/Core/Form/FormState.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------------
  97 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
 110 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
 127 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
 198 | ERROR | Doc comment short description must be on a single line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
-------------------------------------------------------------------------------------------------------------------------------------------------------------

Other than those this change looks good.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new diff-3-8.txt5 KB
new 3268829-8.patch35.64 KB

@bbrala, thanks for the review!

The patch needed a reroll and I did both the reroll and the changes for #7 in this patch. The changes for the reroll were in \Drupal\views\Plugin\views\query\Sql.

bbrala’s picture

bbrala
Location Netherlands
commented

Could you please attach an interdiff, not a diff, kinda unreadable and means there might be parts missing.

bbrala’s picture

bbrala
Location Netherlands
commented

If you want to know why, we have great docs on that :)

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

I've went through the new changes, those look good. Interdiff is failing, probably because of the rebase. But the diff makes sense.

Setting to RTBC :) Thanks for makin this happen.

quietone’s picture

quietone commented
StatusFileSize
new diff-3-8-v2.txt11.54 KB
new interdiff-3-8.txt2.33 KB

Thanks!

Yes, and interdiff is preferred. However, that often fails with a rerolll. When rerolling and the interdiff has failures I create a diff. Although I don't usually use the '-u' option as specified in the instructions as What about rerolled patches?. I have done that now and am uploading a new diff.

I am also uploading the interdiff which has a failure.

Cheers ;-)

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff-8-13-9.5.x.txt630 bytes
new 3268829-13-9.5.x.patch35.63 KB

The patch applies to 10.1.x but not 9.5.x. Here is a patch for 9.5.x.

bbrala’s picture

bbrala
Location Netherlands
commented

The change is really minimal. Inteerdiff is looking good, patch at those files also looks good. Therefor i will RTBC if testrun is green.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

Rtbc. Just to note, my inspection was visual since the dif was so minimal.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3268829-13-9.5.x.patch, failed testing. View results

quietone’s picture

quietone commented

Test fail seems unrelated to these cs fixes, rerunning tests.

1) Drupal\Tests\views_ui\FunctionalJavascript\LibraryCachingTest::testConsecutiveDialogRequests
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-150606:9515/session/c8e259ea0f144a1e741a96e2e210e771/moveto with params: {"element":"0.22354884482981108-1"}
quietone’s picture

quietone commented
Status: Needs work » Needs review

Tests passing, back to NR

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

Oops, that should have been back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 5b84c09246 to 10.1.x and d6f7236a17 to 10.0.x. Thanks!
Committed db0a7b4 and pushed to 9.5.x. Thanks!

Backported to 9.5.x as this is a documentation only patch.

  • alexpott committed 5b84c09 on 10.1.x 
    Issue #3268829 by quietone, bbrala: Fix class property comments for...

  • alexpott committed d6f7236 on 10.0.x 
    Issue #3268829 by quietone, bbrala: Fix class property comments for...

  • alexpott committed db0a7b4 on 9.5.x 
    Issue #3268829 by quietone, bbrala: Fix class property comments for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.