rubocops/lines: require a comment for skip_clean

[branchvincent]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

Related to the discussion in Homebrew/homebrew-core#176257, I didn't realize we expected skip_clean to be documented with a comment. This formalizes that requirement into an audit, scoped only to homebrew/core.

[issyl0]

💪🏻 Thanks!

[issyl0]

nit, unless nil confuses me :-)

Suggested change

	return unless method_comment.nil?
	return if method_comment.present?

[issyl0]

Or

Suggested change

	return unless method_comment.nil?
	return if !method_comment.nil?

[MikeMcQuaid]

I prefer the prior or the existing version to the second.

[MikeMcQuaid]

One optional style nit from @issyl0 and should be good to go, thanks @branchvincent!

@Homebrew/core @Homebrew/maintainers any thoughts on other DSL elements that should always have a comment above them?

[p-linnane]

Off the top of my head: patches should have an explanation of what they fix and when they can be removed.

[Bo98]

Yeah patches is something I campaigned on every PR while back to have comments to the point it seems to be common maintainer knowledge now.

Other cases are largely situational. E.g. if someone added a compiler flag workaround I'd also ask to add a comment (though can't really RuboCop check that).

pour_bottle? is however one I'd consider adding.

In all cases, be aware to check for and skip through parent on_system blocks as we can comment it this way:

# comment explaining horrible hack
on_macos do
  on_intel do
    pour_bottle? ...
  end
end

which is an acceptable form of documenting.

[p-linnane]

+1 for compiler flags as well.

[MikeMcQuaid]

Thanks folks!

@branchvincent would you be interested in adding patches/pour_bottle?/compiler flags in this or a follow-up PR?

[MikeMcQuaid]

Another thought is e.g. ENV.deparallelize and anything else like ENV.O0 or ENV.O1.

[issyl0]

@branchvincent would you be interested in adding patches/pour_bottle?/compiler flags in this or a follow-up PR?

I'm interested if @branchvincent isn't. :-)

[branchvincent]

@branchvincent would you be interested in adding patches/pour_bottle?/compiler flags in this or a follow-up PR?

Yea I'll add them here!

@issyl0 I can take an initial stab at covering these other DSLs but maybe you could help fill in any gaps I might miss? Like the on_macos blocks might be tricky

Also, I think I'll make this a strict audit to give us time to address all the existing offenses

[MikeMcQuaid]

Yea I'll add them here!

Thanks!

Also, I think I'll make this a strict audit to give us time to address all the existing offenses

Good idea!

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.