Remove the requirement that no blank line follow an inline comment

Thanks for creating this issue.

I agree, allowing a blank line after a comment line does make sense. As you say/cite, @todo comments often just want to mark a place in the code where something could/should be added, and don't refer to the next line of code. I see no value in forbidding this, as it is quite clear and deleting the blank line after the @todo just for the sake of Coder only serves to make the code more consufing.
Likewise, having a overview/summary comment before several separately commented code blocks also is something I'd find useful from time to time, and I can't imagine that this would ever really cause confusion.

However, I'm against allowing comments after the code it refers to – I didn't ever have a problem with comments describing code that comes after them, and lifting this restriction would make things less predictable for others reading the code. Now, comments always precede the code they refer to. Afterwards, it would be something left to personal taste, and thus less predictable, counter-acting exactly what coding standards are there for.
I admit, though, it wouldn't hamper readability by much – but I'd still prefer the uniform style we currently have.

Given that comments are written above the code they're appropriate for, should this example:

  public function __construct($module) {
    // Not protected ! Because parent requires public.

not be better written like this?

  /**
   * Not protected because parent requires public.
   */
  public function __construct($module) {

I've done similar things to the following:

$some_code;
$more_code;
 // @todo Remark on the above code and what is still todo.

However, I've written them as follows:

// This does something or other.
// @todo Remark on the above code and what is still todo.
$some_code;
$more_code;

i.e. the @todo comment still goes above the code it's related to.

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

+1. I support this change. Often I want to mark a block of code in this way:

// Start: Part forked from \Drupal\module\SomeClass.
$a = "...";
foreach (...) {
  ...
}
$b = "...";
// End: Part forked from \Drupal\module\SomeClass.

$c = '...';

Sometimes I use an inline comment to give an overview of a section of complex code.

Eg:

function picnic() {
(various bits of picnic prep code)

// Now we need to make the sandwiches. This requires bread, mayo, cheese, and ham.

// Slice the bread.
(several lines of bread code)

// Spread the mayo.
(more lines)

// Get the cheese
(more lines)

// Finally, the ham
(more lines)

I want the sandwiches comment to be separate from the bread comment.

+1 I have also had cases where it is useful to have an empty line following a comment.

Proposal:

Change the next paragraph from Drupal standards for in-line code comments:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line immediately before the code line or block they reference.

with:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line. They are placed immediately before the code line or block they reference, if they refer to a specific code line ot block.

I like that proposal, but the last line is a bit difficult for me, how about we switch the cause and effect:

If the comment refers to a specific code line or block, they should be placed immediately before it.

how about we switch the cause and effect

Love it ❤️

(updated the description)

> If the comment refers to a specific code line or block, they should be placed immediately before it.

Grammar nitpic: 'the comment' is singular, but 'they' is plural.

Other than that, +1 from me.

If the comment refers to a specific code line or block, it should be placed immediately before that code line or block.

This one?

Yup!

Updated the IS with the agreed proposal:

Change the next paragraph from Drupal standards for in-line code comments:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line immediately before the code line or block they reference.

with:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line. If the comment refers to a specific code line or block, it should be placed immediately before that code line or block.

You know... I don't think that either the original or the new version specifically addresses the "with no blank line in between" part of the coding standards. I think that the new version should explicitly say when a blank line is allowed and when it isn't.

Also, if you look at the original page, there is an example immediately after this paragraph, which doesn't have a blank line. Do we need an example that allows a blank line?

We cannot anticipate all the cases. Just looking in this issue description and comments we see already a lot. Trying to define all those in a policy text it's just too much. But we can give at least one example:

Initial version:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line immediately before the code line or block they reference. For example:

// Unselect all other contact categories.
db_query("UPDATE {contact} SET selected = 0");

To be changed with:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line. If the comment refers to a specific code line or block, it should be placed immediately before that code line or block.

Example of inline comment referring a specific line of code:

// Unselect all other contact categories.
db_query("UPDATE {contact} SET selected = 0");

Example of inline comment that doesn't refer a specific line of code or block:

// Hook implementations.

/**
 * Implements hook_form_alter().
 */
function foo_form_alter(array &$form, FormStare $form_state) {
   ...
}

Hm... I am not in favor of having // comments outside of functions. I thought the purpose was for comments within functions/methods, to describe a larger block of code that might contain blank lines? So I don't think I like the example, and I don't think we should allow comments like "Hook implementations.", which also violates our rule that all comments be written in complete sentences.

Also, the proposal in #18 still doesn't say "without a blank line in between", which I think it should.

Also... If we cannot agree on which comments are allowed to have a blank line after them, and which ones are not, then the standard isn't really a standard and will just lead to arguments.

Inline comments are there to help developers and reviewers to understand the code. If the standard will prevent that then we should drop the standard. I really think that we try to over-standardize in an area where we should be more flexible. The standards already in place about comments should be enough.

I'm not a native English speaker so my involvement here is limited.

Normally I'm all for stricter rules and less exceptions, but I agree that we're overdoing it a bit in this case. It's a good thing when someone puts comments in their code, they should be free to separate them as needed if it makes sense.

Also... If we cannot agree on which comments are allowed to have a blank line after them, and which ones are not, then the standard isn't really a standard and will just lead to arguments.

For this issue, I would even say just dropping the "no empty line after an inline comment" rule in general would also be fine. But I guess it's true, when the comment refers to the code immediately following it, we do want people to put the comment in a specific place, so we do want the rule and should just add specific exceptions.

Therefore, my suggestion: Replace this

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line immediately before the code line or block they reference. For example:

// Unselect all other contact categories.
db_query("UPDATE {contact} SET selected = 0");

with this:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don't want to try and describe that", you need to comment it before you forget how it works. Comments should be on a separate line immediately before the code line or block they reference (with no empty line in between). Having an empty line after an inline comment is allowed in cases where a comment doesn't directly refer to existing code (for example, an @todo comment as a placeholder for future code), or should be separated from a subsequent, more specific comment (for example, one comment that describes the whole algorithm and then a comment for the first line(s)). For example:

  public function towerOfHanoi($n, $from, $to) {
    // @todo Do we need to validate parameters first?

    // Nothing to do if we want to move 0 pegs.
    if ($n == 0) {
      return;
    }

    // This implements the recursive solution. See
    // https://en.wikipedia.org/wiki/Tower_of_Hanoi#Recursive_solution.

    // Move n-1 pegs to the spare tower.
    $spare = 3 - $from - $to;
    $this->towerOfHanoi($n - 1, $from, $spare);

    // Move the bottom peg to the destination.
    $this->moveSinglePeg($from, $to);

    // Move the n-1 pegs from the spare tower to the destination.
    $this->towerOfHanoi($n - 1, $spare, $to);
  }

Hm, or is the example code too lengthy? I kinda drew a blank trying to think of a two-step process. I guess we can also go with "We first need to do one thing and then another.", $this->doOneThing(), etc.

I hit another use-case:

    $this->clickLink('Cancel');
    // @todo Uncomment the following line after Core issue is fixed. See
    // https://www.drupal.org/project/drupal/issues/2582295.
    // $this->assertSession()->addressEquals($overview_url);

    // Go to the next page.
    $this->drupalGet($next_page);

This is currently flagged as a Coding Standards violation, but is yet another completely legitimate usecase of a blank line following an inline comment (resp. a commented out code line).

Another use case is folding regions.
https://blog.jetbrains.com/phpstorm/2012/03/new-in-4-0-custom-code-foldi...

RTBC for #21
It defines very clear when to use inline comments and when not to.

Here's another example where this is needed. Documenting a switch case fall-through cannot be done with the current standards.

I have something like this

$i = 0;
switch ($var) {
  case 'first':
    $i = 1;

  case 'second':
  default:
    return myfunction($i);
}

The problem is how to document the fall-through? I would like to put a code comment just after the $i =1, like this:

    $i = 0;
    switch ($var) {
      case 'first':
        $i = 1;
        // Fall-through to default case.

      case 'second':
      default:
        return myfunction($i);
    }

But I can't do that because There must be no blank line following an inline comment
An alternative would be:

    $i = 0;
    switch ($var) {
      case 'first':
        $i = 1;

      // Fall-through to default case.
      case 'second':
      default:
        return myfunction($i);
    }

But then I'm told that the comment is not indented far enough: Line indented incorrectly; expected 8 spaces, found 6

Obviously this code has been simplified, so please assume that the actual code is reasonable and necessary - I'm not looking for ways to rewrite this, just a way to DOCUMENT the fall-through.

Both of these make sense, too, yes. However, I’d say they are more or less covered by the “doesn’t directly refer to existing code” case already. Or would you disagree? We could also put the “existing” in parantheses to make it clearer, though that gives the sentence a funny look.

Anyways, I updated my proposal from #21 a bit, breaking the long paragraph in two and shortening the example somewhat, and added it to the IS, so people will know where the discussion currently stands. Here the proposed text is again:

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don’t want to try and describe that", you need to comment it before you forget how it works.

Comments should be on a separate line immediately before the code line or block they reference (with no empty line in between). Having an empty line after an inline comment is allowed in cases where a comment doesn’t directly refer to existing code (for example, a @todo comment as a placeholder for future code), should be separated from a subsequent, more specific comment (for example, one comment that describes the whole algorithm and then a comment for the first line(s)). For example:

  public function towerOfHanoi($n, $from, $to) {
    // @todo Do we need to validate parameters first?

    // Nothing to do if we want to move 0 pegs.
    if ($n == 0) {
      return;
    }

    // This implements the recursive solution. See
    // https://en.wikipedia.org/wiki/Tower_of_Hanoi#Recursive_solution.

    // Move n-1 pegs to the spare tower.
    $spare = 3 - $from - $to;
    $this->towerOfHanoi($n - 1, $from, $spare);
    // […]
  }

However, I don’t think we have had any new standards accepted in years now (looks like three years, in fact, at a quick glance), so just building support (which seems pretty strong already anyways) likely won’t matter much. We’d have to finally get the Coding Standards process working again. (Again.)

Still, should probably tag it appropriately.

This has been RTBC for coming up to three years. Is the process of adopting standards actually being followed or have we stalled?

It has been comatose for a long time, but there is recently renewed energy again, with a bi-weekly Slack meeting for going through issues. So, maybe/hopefully this will also get resolved in the near future. Please think about participating if you’re interested in coding standards.

Update the IS with a template I am starting to use.

In doing so I read the proposed changes. This sentence is very hard to read and English is my first language.

Having an empty line after an inline comment is allowed in cases where a comment doesn’t directly refer to existing code (for example, a @todo comment as a placeholder for future code), should be separated from a subsequent, more specific comment (for example, one comment that describes the whole algorithm and then a comment for the first line(s)).

I suggest something along these lines. Yes, I do know that this form changes the scope to two situations.

An empty line after an inline comment is allowed in two case.

1. A @todo comment as a placeholder for future code.
2. To separate a summary of the whole algorithm from more specific comments.

+1 to #29, much clearer!

To separate a summary of the whole algorithm from more specific comments.

I think the two cases in #29 are good.
But a sniff is not going to be able to make this judgement. Is this rule going to be turned off entirely or is the idea then just to live with a certain number of false positives? The latter has always caused problems for me, because there are a lot of people who will just run phpcs and submit patches that do all sorts of wrong things just so they can eliminate all the warnings - it's no longer about improving the code, it's all about making the warnings go away even if it makes the code worse. I don't need more of that.

The sniff can indeed not determine the content of the comment, so we should just allow blank spaces between subsequent comments. We should still detect a blank line that separates a comment from code.

Allowed:

// Some comment.

// Some comment.
$node = $this->storage->load($nid);

// @todo Some technical debt.

// Some comment.
$node = $this->storage->load($nid);

Not allowed:

// Some comment.

$node = $this->storage->load($nid);

// @todo Some technical debt.

$node = $this->storage->load($nid);

For simplicity, we should probably just ignore the presence of todos in the sniff. We could in theory detect that a @todo should precede a comment and not the other way around, but I think this is not worth the effort and would probably just cause confusion / irritation for developers who are not super familiar with the coding standards.

I agree with TR and with several others who have commented above - I think we just need to turn off this rule completely. There are many examples given in this issue and in the Coder issue #2159253: Coder - alter requirements about no blank line following an inline comment where inline comments benefit from having a blank beneath, or being after a block of code. It's not just the two cases mentioned in #29. It would be impossible for the sniff to be enhanced to try to capture all the scenarios we have been talking about, and not produce many false positives.

We all agree that commenting is vital. Anything that makes commenting harder to do, or makes it less understandable by the next developer to maintain the code, has to be a bad thing.

The coding standards policy can be updated to give recommendations on best practices, that is still a good idea. But it is also fine that we do not have Coder sniffs that attempt to enforce these suggestions.

[edit: my comment overlapped with pfrenssen #32, I did not see that when I submitted.]

I agree with @pfrenssen in #32: allow blank spaces between subsequent comments

Disabling the sniff completely will results in a lot of hanging comments.

This seems like an expected thing to do in some cases, where a comment may be helpful context but not directly related to the immediately following code, otherwise it's forcing a potentially trivial or unhelpful comment on the following code:

// Some comment.

$node = $this->storage->load($nid);

This would align with reason 1 in #29 (or at least it would not be possible to distinguish between a todo to add code, or a todo for the following lines):

// @todo Some technical debt.

$node = $this->storage->load($nid);

I don't think we're risking a wave of detached comments by not having a sniff (that may result in a lot of false positives).

----

I don't think specifically enumerating exceptions is helpful

Non-header or in-line comments are strongly encouraged. A general rule of thumb is that if you look at a section of code and think "Wow, I don’t want to try and describe that", you need to comment it before you forget how it works.

Inline comments should be on a separate line immediately before the code line or block they reference with no empty line in between.

  // Unselect all other contact categories.
  db_query("UPDATE {contact} SET selected = 0");

When a comment doesn’t directly refer to existing code it should be separated from subsequent comments or code with an empty line. Some examples are:
- A @todo comment as a placeholder for future code.
- To separate a summary of a code block from more specific comments.
- To document when a case in a switch statement has omitted a break intentionally to fall through to the following case.

public function towerOfHanoi($n, $from, $to) {
    // @todo Do we need to validate parameters first?

    // Nothing to do if we want to move 0 pegs.
    if ($n == 0) {
      return;
    }

    // This implements the recursive solution. See
    // https://en.wikipedia.org/wiki/Tower_of_Hanoi#Recursive_solution.

    // Move n-1 pegs to the spare tower.
    $spare = 3 - $from - $to;
    $this->towerOfHanoi($n - 1, $from, $spare);

    // Move the bottom peg to the destination.
    $this->moveSinglePeg($from, $to);

    // Move the n-1 pegs from the spare tower to the destination.
    $this->towerOfHanoi($n - 1, $spare, $to);
  }

i am reviewing this to get it ready for announcement. According the current process this needs to have a working patch to test the change.

All changes that can be tested with coder/phpcs. A working patch is to be provided with the issue. Committers will test the phpcs rule as part of the approval process.

And the sniff should handle the case pointed out in #32, "detect a blank line that separates a comment from code".

There is also a current core patch to enable the existing sniff, #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard. If this is approved, that sniff will not be implemented in core. What happens to the sniff itself?

I think that #35 raised a valid point about listing exceptions so I updated the Issue Summary with their suggestion.

Setting to needs work for having a patch to test this with.

Regarding the coder sniff we should look to be able to modify the sniff so that it can still be used. If that is the case then the core issue #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard should be put on hold, and postponed until (a) we finalise the updated standard and then (b) Coder issue #2159253: Coder - alter requirements about no blank line following an inline comment has been fixed to enhance the sniff and (c) a new Coder release is made availbale for Core to use.

It seems that we are heading for agreement on the proposals, and below is a summary of the cases. This does not contradict @quietone's update of the IS.

The following are all acceptable

// Case 1 - a summary paragraph about the following code
// It describes the whole process.

// Now to start the work.
$foo = bar()

// Case 2 - single line titles are OK.

// Case 3 - This is a second paragraph, it is also allowed. Here we talk
// about the following code and describe the whole process.

// The process begins.
$foo = bar()

The following is still not allowed

// A comment with a blank line but no subsequent comment for code lines.

$foo = bar()

To summarise this in words: "An inline comment should be followed immediatey by a code line or a blank line followed by another comment". If we agree that this is what the standard is then we can modify the sniff the test for this, and hence keep the sniff active in contrib and core, and prevent floating comments unattached to anything.

As a trial of the new template proposed in #3387167: Add an issue template for the Coding Standards project I have converted the issue summary here to use the possible new template, to see how it looks. This may also provide feedback for the template issue.

I realised it was wrong for me to add the original issue raiser (webel) as a supporter in retrospect. Supporters should be added in 'real time' and also only by themselves, not others.

So can we have two more supporters please? Then we can progress on to the next step.

Adding myself as supporter.

Added support. Summary paragraphs are very useful!

Thanks for adding the support. Step 3 done. I have created a draft CR

What is the reason we're not also allowing the "A comment with a blank line but no subsequent comment for code lines." case?

I think #35 gave a good examples, but disallowing this case specifically might cause developers to add too many comments. I understand we want to have tooling that guides people in the right direction. However, writing useful documentation and comments is more art than science which makes capturing it in a rule difficult.

As a counter example:

  public function assertUserHasPermissin($account, $permission) {
    // @todo Implement deprecation warnings on our way to type-hinted permissions.

    if (!$account->hasPermission($permission)) {
     throw SomeException();
    }
  }

Given the proposed rule, this would be invalid. I think having the @todo separated is clearer than the if-statement because it isn't related to the if-statement (that's the whole point of the issue). However, a sniff to disallow this would cause a developer to likely add something to the if-statement such as "Check that the account has the provided permission." which I would argue is actively harmful, because it doesn't provide any information the code itself does not provide:

• It requires a user to read the comment and then read the code, essentially reading the same thing twice
• The risk of the code deviating from the comment is now much higher, because the rule encouraged a developer to comment where it might not be needed. In that case it's a lot easier to comment "action" rather than "intent", which is a less useful comment

My proposal would be to let go of that restriction too.

OK, how about if we made an exception for @todo comments? That would satisfy your requirement here. I think @todo are a special case, so it would be OK for them not to have a code line or another comments. What we do not want is floating comments of any random kind.