While it doesn't necessarily follow the strict letter of the guideline, it would be nice if a comment could end with a closing parenthesis, e.g.

function views_ui_add_limited_validation($element, FormStateInterface $form_state) {
  // Retrieve the AJAX triggering element so we can determine its parents. (We
  // know it's at the same level of the complete form array as the submit
  // button, so all we have to do to find it is swap out the submit button's
  // last array parent.)

 // Look through the child properties to find the data reference
 // property that should be the "real field" for the relationship.
 // (For Core entity references, this will usually be ":entity".)

Currently you'll get complaints for both the DocCommentSniff and the InlineCommentSniff (at least that I've noticed), but it's valid English and I think it follows the spirit of the guideline.

CommentFileSizeAuthor
#18 allow_comment_to_end_in-2683691-18.patch1.96 KBborisson_
#16 allow_comment_to_end_in-2683691-16.patch1.61 KBborisson_
#12 interdiff.txt592 byteskevin.dutra
#12 comment-end-parenthesis-2683691-12.patch2.39 KBkevin.dutra
#10 allow_comment_to_end_in-2683691-10.patch2.33 KBborisson_
#10 interdiff.txt696 bytesborisson_
#3 comment-end-parenthesis-2683691-3.patch1.65 KBkevin.dutra

Comments

kevin.dutra created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Strongly in favor of that, having to rewrite it is ridiculous.
Either a closing paranthesis should always be allowed, or there needs to be a clever way to check for this exception.

kevin.dutra’s picture

kevin.dutra commented
Status: Active » Needs review
StatusFileSize
new comment-end-parenthesis-2683691-3.patch1.65 KB

Here's a simple change to allow the closing paren.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Postponed (maintainer needs more info)

Could you provide some real world comment examples in the issue summary? I'm not convinced by the example.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Issue summary: View changes

I added a couple of places where we are having issues with this rule in search api.

kevin.dutra’s picture

kevin.dutra commented
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs review

Replaced my original sample with an example out of core (8.0.5).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes
Status: Needs review » Active

OK, removed one example that does not end in a ")".

I think the "(...)" part of a comment should be the last part of a sentence followed by a dot, but I guess people might want to do it differently.

Anyone want to supply a patch to allow ")" as ending character?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

@klausi, is the patch in #3 not sufficient?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Needs work
Issue tags: +Needs tests

Ah, sorry, overlooked the patch. Almost ready, it only needs a test case in good.php.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt696 bytes
new allow_comment_to_end_in-2683691-10.patch2.33 KB
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

phpunit is failing:

1) Drupal_GoodUnitTest::testSniff
[LINE 1265] Expected 0 warning(s) in good.php but found 1 warning(s). The warning(s) found were:
 -> There must be no blank line following an inline comment (Drupal.Commenting.InlineComment.SpacingAfter)
[LINE 1269] Expected 0 error(s) in good.php but found 1 error(s). The error(s) found were:
 -> Doc comment short description must be on a single line, further text should be a separate paragraph (Drupal.Commenting.DocComment.ShortSingleLine)
kevin.dutra’s picture

kevin.dutra commented
Status: Needs work » Needs review
StatusFileSize
new comment-end-parenthesis-2683691-12.patch2.39 KB
new interdiff.txt592 bytes

I think this should button up those couple items.

  • klausi committed e8315ad on 8.x-2.x authored by kevin.dutra 
    Issue #2683691 by kevin.dutra, borisson_: Allow comment to end in a...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Committed, thanks!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Awesome, thanks @klausi!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Fixed » Needs review
StatusFileSize
new allow_comment_to_end_in-2683691-16.patch1.61 KB

Not sure if I should've opened another issue for this, but this is also valid for parameter comments.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Needs work

Something aweful seems to have happened to your patch there.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
StatusFileSize
new allow_comment_to_end_in-2683691-18.patch1.96 KB
anoopjohn’s picture

anoopjohn commented

Shouldn't the following be the cases

Good

1) This is a sentence with some text in bracket at the end and a full stop after that (this is the text in the bracket).
2) This is a first sentence with a full stop at its end. (This is a full sentence inside a bracket with full stop inside the bracket.)

Bad

1) This is a sentence with text in the bracket at the end but not ending in a full stop after the bracket (this is the text in the bracket)
2) This is a first sentence with a full stop at its end. (This is a full sentence inside a bracket but no full stop)
3) This is a first sentence without a full stop (This is a full sentence inside a bracket but no full stop)

I do not think we will be able to detect if the text before the bracket is a full sentence or not. Same for the text inside the bracket.

So the cases we can programmatically test are

a) If there is no full stop just before the opening bracket then there should be a full stop after the closing bracket.
b) If there is a full stop just before the opening bracket then there should be a full stop inside the closing bracket or outside the closing bracket

Auto correct could be

a) If there is no full stop just before the opening bracket and there is no full stop after the closing bracket then add a full stop after the closing bracket.
b) If there is a full stop just before the opening bracket and there is no full stop inside the closing bracket or outside the closing bracket then add a full stop after the closing bracket.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

b) If there is a full stop just before the opening bracket then there should be a full stop inside the closing bracket or outside the closing bracket

b) If there is a full stop just before the opening bracket and there is no full stop inside the closing bracket or outside the closing bracket then add a full stop after the closing bracket.

That's invalid English grammar. If the whole sentence is inside the parentheses, why would you have the terminating period outside it? (It also looks silly).

But of course, checking for a stop (or exclamation/question mark) before the closing parenthesis (or parentheses, as may be the case) would make sense, and the results probably even more reliable. It would just be much more work, so I think first avoiding false positives is a sensible decision, before maybe improving this rule further.

  • klausi committed e4cd232 on 8.x-2.x authored by borisson_ 
    Issue #2683691 by borisson_: Allow @param comment to end in a closing...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Agreed with drunken monkey.

Committed the patch, thanks!

Status: Fixed » Closed (fixed)

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

anoopjohn’s picture

anoopjohn commented

Documenting this here

https://www.drupal.org/drupalorg/style-guide/content#punctuation

From https://www.drupal.org/node/1354#drupal

All documentation and comments should form proper sentences, use proper grammar and punctuation, and generally follow the same style guidelines as Drupal.org content: http://drupal.org/style-guide/content

We may have to re-look at the autocorrect for periods when there are closing brackets involved. The standards say that closing brackets should either have a period before it or after it if it is the last character in a sentence.