rubocop: Trim `Naming/MethodParameterName` allowlist #14922

issyl0 · 2023-03-08T00:46:11Z

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?

This was a TODO. So I did some of it. Not gone through all of them yet, but this is a significantly shorter list.

BrewTestBot · 2023-03-08T00:46:26Z

Review period will end on 2023-03-09 at 00:46:11 UTC.

MikeMcQuaid

Great work here! One suggested tweak only to a comment.

MikeMcQuaid · 2023-03-08T12:44:52Z

Library/.rubocop.yml

@@ -183,7 +183,6 @@ Naming/MethodName:
    - '\A(fetch_)?HEAD\?\Z'

 # allow those that are standard
-# TODO: try to remove some of these


I think we can leave this comment as-is and aim to remove more later.

I'm personally of the belief that single/double letter variables almost always hinder readability more than the value they provide through speedy typing.

The only one I can see being ever valuable here is f and pr because their meanings are more obvious but even f has been widely misused to mean f(ormula or cask) which hinders readability.

Yeah I was gonna follow up with this, I just got tired last night. Shouldn't have removed the comment. 💤

I think there's an argument that pr as an allowed variable should be upstreamed to core RuboCop given it's so ubiquitous, but I'll try that another day.

@issyl0 Yeh, I'm a strong 👍🏻 on "if we can get Exclude: and/or Enabled: false changes upstreamed then let's do so and otherwise: let's try to move towards a configuration RuboCop supports".

Library/.rubocop.yml

MikeMcQuaid

Thanks @issyl0!

MikeMcQuaid · 2023-03-08T12:53:42Z

Good to 🚢 once merge conflicts fixed and CI is 💚

BrewTestBot · 2023-03-08T12:53:45Z

Review period skipped due to critical label.

- These are already included in https://github.com/rubocop/rubocop/blob/6136ffd91e8d6fba22d6fb44ec8498a5d43537d0/config/default.yml#L2834-L2862 upstream default allowed method names, so we don't need them here too. - Practically this makes no difference since the `inherit_mode` is `merge` it'll just merge the two, but for tidiness I thought I'd do this anyway since the duplication annoyed me.

- This was an easy fix, "v" => "version".

- I originally thought this was short for "function", but upon closer inspection all its usages are to do with filenames. So, use "filename", it's clearer.

- It's entirely unused as a parameter name.

- Most usages of this were in the `pretty_duration` method, where "s" is better described as "seconds" since we're calculating a duration. - I also took the executive decision to do the same to "m" which refers to "minutes".

- This stands for "resource" in our case, so use the long name.

- It's used to refer to a patch, so use the long name.

- It seems that "ff" was short for plural formula, so formulae.

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

MikeMcQuaid · 2023-03-08T15:00:24Z

Library/Homebrew/formula_text_auditor.rb

+    def include?(str)
+      @text.include? str


Suggested change

def include?(str)

@text.include? str

def include?(string)

@text.include? string

MikeMcQuaid · 2023-03-08T15:00:44Z

Library/Homebrew/utils/formatter.rb

@@ -50,14 +50,14 @@ def error(string, label: nil)
  # so we always wrap one word before an option.
  # @see https://github.com/Homebrew/brew/pull/12672
  # @see https://macromates.com/blog/2006/wrapping-text-with-regular-expressions/
-  def format_help_text(s, width: 172)
+  def format_help_text(str, width: 172)


Suggested change

def format_help_text(str, width: 172)

def format_help_text(string, width: 172)

MikeMcQuaid · 2023-03-08T15:01:01Z

Library/Homebrew/utils/formatter.rb

-     .gsub(/^( +-.+  +(?=\S.{#{desc}}))(.{1,#{desc}})( +|$)(?!-)\n?/, "\\1\\2\n#{" " * indent}")
-     .gsub(/^( {#{indent}}(?=\S.{#{desc}}))(.{1,#{desc}})( +|$)(?!-)\n?/, "\\1\\2\n#{" " * indent}")
-     .gsub(/(.{1,#{width}})( +|$)(?!-)\n?/, "\\1\n")
+    str.gsub(/(?<=\S) *\n(?=\S)/, " ")


Suggested change

str.gsub(/(?<=\S) *\n(?=\S)/, " ")

string.gsub(/(?<=\S) *\n(?=\S)/, " ")

and this will break indentation below, sorry!

Automerge was too quick! I'll fix these up soon!

No worries, I triggered it 😅

- Three characters is the RuboCop limit for parameter names, but more descriptive is good. - Requested in Homebrew#14922 (review), but the automerge was too quick for me to get to it.

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 8, 2023

MikeMcQuaid approved these changes Mar 8, 2023

View reviewed changes

MikeMcQuaid reviewed Mar 8, 2023

View reviewed changes

Library/.rubocop.yml Outdated Show resolved Hide resolved

MikeMcQuaid approved these changes Mar 8, 2023

View reviewed changes

MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Mar 8, 2023

MikeMcQuaid enabled auto-merge March 8, 2023 12:53

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 8, 2023

BrewTestBot approved these changes Mar 8, 2023

View reviewed changes

issyl0 and others added 10 commits March 8, 2023 14:40

rubocop: Drop "v" from Naming/MethodParameterName allowlist

3e7a46f

- This was an easy fix, "v" => "version".

rubocop: Drop "fn" from Naming/MethodParameterName allowlist

6d822d9

- I originally thought this was short for "function", but upon closer inspection all its usages are to do with filenames. So, use "filename", it's clearer.

rubocop: Drop "rb" from Naming/MethodParameterName allowlist

87042c0

- It's entirely unused as a parameter name.

rubocop: Drop "s" from Naming/MethodParameterName allowlist

89fb8c7

- Most usages of this were in the `pretty_duration` method, where "s" is better described as "seconds" since we're calculating a duration. - I also took the executive decision to do the same to "m" which refers to "minutes".

rubocop: Drop "r" from Naming/MethodParameterName allowlist

0cd616b

- This stands for "resource" in our case, so use the long name.

rubocop: Drop "p" from Naming/MethodParameterName allowlist

e273c25

- It's used to refer to a patch, so use the long name.

rubocop: Drop "d" from Naming/MethodParameterName allowlist

a3211f4

rubocop: Drop "ff" from Naming/MethodParameterName allowlist

f52fbaf

- It seems that "ff" was short for plural formula, so formulae.

rubocop: Reinstate TODO for Naming/MethodParameterName

e1c4555

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

issyl0 force-pushed the rubocop-naming-method-parameter-name branch from a727a69 to e1c4555 Compare March 8, 2023 14:43

MikeMcQuaid merged commit 9937681 into Homebrew:master Mar 8, 2023
22 of 23 checks passed

MikeMcQuaid reviewed Mar 8, 2023

View reviewed changes

issyl0 deleted the rubocop-naming-method-parameter-name branch March 8, 2023 15:15

issyl0 mentioned this pull request Mar 8, 2023

Make str variable names longer #14927

Merged

7 tasks

github-actions bot added the outdated PR was locked due to age label Apr 8, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rubocop: Trim `Naming/MethodParameterName` allowlist #14922

rubocop: Trim `Naming/MethodParameterName` allowlist #14922

issyl0 commented Mar 8, 2023

BrewTestBot commented Mar 8, 2023

MikeMcQuaid left a comment

MikeMcQuaid Mar 8, 2023

issyl0 Mar 8, 2023

issyl0 Mar 8, 2023

MikeMcQuaid Mar 8, 2023

MikeMcQuaid left a comment

MikeMcQuaid commented Mar 8, 2023

BrewTestBot commented Mar 8, 2023

MikeMcQuaid Mar 8, 2023

MikeMcQuaid Mar 8, 2023

MikeMcQuaid Mar 8, 2023

issyl0 Mar 8, 2023

MikeMcQuaid Mar 8, 2023

	def format_help_text(str, width: 172)
	def format_help_text(string, width: 172)

	str.gsub(/(?<=\S) *\n(?=\S)/, " ")
	string.gsub(/(?<=\S) *\n(?=\S)/, " ")

rubocop: Trim Naming/MethodParameterName allowlist #14922

rubocop: Trim Naming/MethodParameterName allowlist #14922

Conversation

issyl0 commented Mar 8, 2023

BrewTestBot commented Mar 8, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 8, 2023

BrewTestBot commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubocop: Trim `Naming/MethodParameterName` allowlist #14922

rubocop: Trim `Naming/MethodParameterName` allowlist #14922