Skip to content

Commit

Permalink
Replace Rails Blank/Present cops
Browse files Browse the repository at this point in the history
  • Loading branch information
dduugg committed Jan 19, 2024
1 parent a1ce57b commit 9c6dbaa
Show file tree
Hide file tree
Showing 7 changed files with 363 additions and 9 deletions.
14 changes: 7 additions & 7 deletions Library/.rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ FormulaAuditStrict:
Homebrew:
Enabled: true

Homebrew/Blank:
Exclude:
# Core extensions are not available here:
- "Homebrew/rubocops/**/*"

# only used internally
Homebrew/MoveToExtendOS:
Enabled: false
Expand Down Expand Up @@ -209,17 +214,10 @@ Rails:
- "Homebrew/rubocops/**/*"

# These relate to ActiveSupport and not other parts of Rails.
Rails/Blank:
Enabled: true
Rails/CompactBlank:
Enabled: true
Rails/Presence:
Enabled: true
Rails/Present:
Enabled: true
Exclude:
# `present?` is defined as `!blank?` wihin this file
- "Homebrew/extend/blank.rb"
Rails/SafeNavigationWithBlank:
Enabled: true

Expand Down Expand Up @@ -358,6 +356,7 @@ Style/InverseMethods:
- "Homebrew/standalone/init.rb"
- "Homebrew/rubocops/**/*"
InverseMethods:
:blank?: :present?
:exclude?: :include?

Style/InvertibleUnlessCondition:
Expand All @@ -369,6 +368,7 @@ Style/InvertibleUnlessCondition:
:zero?:
# Don't require non-standard `exclude?` (for now at least) - it's not available in every file
:include?:
:blank?: :present?

Style/MutableConstant:
# would rather freeze too much than too little
Expand Down
2 changes: 2 additions & 0 deletions Library/Homebrew/rubocops/all.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
# frozen_string_literal: true

require_relative "../extend/array"
require_relative "blank"
require_relative "io_read"
require_relative "move_to_extend_os"
require_relative "present"
require_relative "shell_commands"

# formula audit cops
Expand Down
74 changes: 74 additions & 0 deletions Library/Homebrew/rubocops/blank.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# typed: true
# frozen_string_literal: true

module RuboCop
module Cop
module Homebrew
# Checks for code that can be written with simpler conditionals
# using `Object#blank?`.
#
# @safety
# This cop is unsafe autocorrection, because `' '.empty?` returns false,
# but `' '.blank?` returns true. Therefore, autocorrection is not compatible
# if the receiver is a non-empty blank string, tab, or newline meta characters.
#
# @example NilOrEmpty: true (default)
# # Converts usages of `nil? || empty?` to `blank?`
#
# # bad
# foo.nil? || foo.empty?
# foo == nil || foo.empty?
#
# # good
# foo.blank?
class Blank < Base
extend AutoCorrector

MSG_NIL_OR_EMPTY = "Use `%<prefer>s` instead of `%<current>s`."
RESTRICT_ON_SEND = [:!].freeze

# `(send nil $_)` is not actually a valid match for an offense. Nodes
# that have a single method call on the left hand side
# (`bar || foo.empty?`) will blow up when checking
# `(send (:nil) :== $_)`.
def_node_matcher :nil_or_empty?, <<~PATTERN
(or
{
(send $_ :!)
(send $_ :nil?)
(send $_ :== nil)
(send nil :== $_)
}
{
(send $_ :empty?)
(send (send (send $_ :empty?) :!) :!)
}
)
PATTERN

def on_or(node)
nil_or_empty?(node) do |var1, var2|
return if var1 != var2

message = format(MSG_NIL_OR_EMPTY, prefer: replacement(var1), current: node.source)
add_offense(node, message: message) do |corrector|
autocorrect(corrector, node)
end
end
end

private

def autocorrect(corrector, node)
variable1, _variable2 = nil_or_empty?(node)
range = node.source_range
corrector.replace(range, replacement(variable1))
end

def replacement(node)
node.respond_to?(:source) ? "#{node.source}.blank?" : "blank?"
end
end
end
end
end
76 changes: 76 additions & 0 deletions Library/Homebrew/rubocops/present.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# typed: true
# frozen_string_literal: true

module RuboCop
module Cop
module Homebrew
# Checks for code that can be written with simpler conditionals
# using `Object#present?`.
#
# @example
# # Converts usages of `!nil? && !empty?` to `present?`
#
# # bad
# !foo.nil? && !foo.empty?
#
# # bad
# foo != nil && !foo.empty?
#
# # good
# foo.present?
class Present < Base
extend AutoCorrector

MSG_EXISTS_AND_NOT_EMPTY = "Use `%<prefer>s` instead of `%<current>s`."

def_node_matcher :exists_and_not_empty?, <<~PATTERN
(and
{
(send (send $_ :nil?) :!)
(send (send $_ :!) :!)
(send $_ :!= nil)
$_
}
{
(send (send $_ :empty?) :!)
}
)
PATTERN

def on_and(node)
exists_and_not_empty?(node) do |var1, var2|
return if var1 != var2

message = format(MSG_EXISTS_AND_NOT_EMPTY, prefer: replacement(var1), current: node.source)

add_offense(node, message: message) do |corrector|
autocorrect(corrector, node)
end
end
end

def on_or(node)
exists_and_not_empty?(node) do |var1, var2|
return if var1 != var2

add_offense(node, message: MSG_EXISTS_AND_NOT_EMPTY) do |corrector|
autocorrect(corrector, node)
end
end
end

def autocorrect(corrector, node)
variable1, _variable2 = exists_and_not_empty?(node)
range = node.source_range
corrector.replace(range, replacement(variable1))
end

private

def replacement(node)
node.respond_to?(:source) ? "#{node.source}.present?" : "present?"
end
end
end
end
end
4 changes: 2 additions & 2 deletions Library/Homebrew/startup/bootsnap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# frozen_string_literal: true

# Disable Rails cops, as we haven't required active_support yet.
# rubocop:disable Rails
# rubocop:disable Rails, Homebrew/Blank
homebrew_bootsnap_enabled = ENV["HOMEBREW_NO_BOOTSNAP"].nil? && !ENV["HOMEBREW_BOOTSNAP"].nil?

# we need some development tools to build bootsnap native code
Expand Down Expand Up @@ -49,4 +49,4 @@
$stderr.puts "Error: HOMEBREW_BOOTSNAP could not `require \"bootsnap\"`!\n\n"
end
end
# rubocop:enable Rails
# rubocop:enable Rails, Homebrew/Blank
108 changes: 108 additions & 0 deletions Library/Homebrew/test/rubocops/blank_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# frozen_string_literal: true

require "rubocops/blank"

describe RuboCop::Cop::Homebrew::Blank do
subject(:cop) { described_class.new }

shared_examples "offense" do |source, correction, message|
it "registers an offense and corrects" do
expect_offense(<<~RUBY, source: source, message: message)
#{source}
^{source} #{message}
RUBY

expect_correction(<<~RUBY)
#{correction}
RUBY
end
end

it "accepts checking nil?" do
expect_no_offenses("foo.nil?")
end

it "accepts checking empty?" do
expect_no_offenses("foo.empty?")
end

it "accepts checking nil? || empty? on different objects" do
expect_no_offenses("foo.nil? || bar.empty?")
end

# Bug: https://github.com/rubocop/rubocop/issues/4171
it "does not break when RHS of `or` is a naked falsiness check" do
expect_no_offenses("foo.empty? || bar")
end

it "does not break when LHS of `or` is a naked falsiness check" do
expect_no_offenses("bar || foo.empty?")
end

# Bug: https://github.com/rubocop/rubocop/issues/4814
it "does not break when LHS of `or` is a send node with an argument" do
expect_no_offenses("x(1) || something")
end

context "when nil or empty" do
it_behaves_like "offense", "foo.nil? || foo.empty?",
"foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of `foo.nil? || foo.empty?`."
it_behaves_like "offense", "nil? || empty?", "blank?", "Homebrew/Blank: Use `blank?` instead of `nil? || empty?`."
it_behaves_like "offense", "foo == nil || foo.empty?",
"foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of `foo == nil || foo.empty?`."
it_behaves_like "offense", "nil == foo || foo.empty?",
"foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of `nil == foo || foo.empty?`."
it_behaves_like "offense", "!foo || foo.empty?", "foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of `!foo || foo.empty?`."

it_behaves_like "offense", "foo.nil? || !!foo.empty?",
"foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of `foo.nil? || !!foo.empty?`."
it_behaves_like "offense", "foo == nil || !!foo.empty?",
"foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of " \
"`foo == nil || !!foo.empty?`."
it_behaves_like "offense", "nil == foo || !!foo.empty?",
"foo.blank?",
"Homebrew/Blank: Use `foo.blank?` instead of " \
"`nil == foo || !!foo.empty?`."
end

context "when checking all variable types" do
it_behaves_like "offense", "foo.bar.nil? || foo.bar.empty?",
"foo.bar.blank?",
"Homebrew/Blank: Use `foo.bar.blank?` instead of " \
"`foo.bar.nil? || foo.bar.empty?`."
it_behaves_like "offense", "FOO.nil? || FOO.empty?",
"FOO.blank?",
"Homebrew/Blank: Use `FOO.blank?` instead of `FOO.nil? || FOO.empty?`."
it_behaves_like "offense", "Foo.nil? || Foo.empty?",
"Foo.blank?",
"Homebrew/Blank: Use `Foo.blank?` instead of `Foo.nil? || Foo.empty?`."
it_behaves_like "offense", "Foo::Bar.nil? || Foo::Bar.empty?",
"Foo::Bar.blank?",
"Homebrew/Blank: Use `Foo::Bar.blank?` instead of " \
"`Foo::Bar.nil? || Foo::Bar.empty?`."
it_behaves_like "offense", "@foo.nil? || @foo.empty?",
"@foo.blank?",
"Homebrew/Blank: Use `@foo.blank?` instead of `@foo.nil? || @foo.empty?`."
it_behaves_like "offense", "$foo.nil? || $foo.empty?",
"$foo.blank?",
"Homebrew/Blank: Use `$foo.blank?` instead of `$foo.nil? || $foo.empty?`."
it_behaves_like "offense", "@@foo.nil? || @@foo.empty?",
"@@foo.blank?",
"Homebrew/Blank: Use `@@foo.blank?` instead of " \
"`@@foo.nil? || @@foo.empty?`."
it_behaves_like "offense", "foo[bar].nil? || foo[bar].empty?",
"foo[bar].blank?",
"Homebrew/Blank: Use `foo[bar].blank?` instead of " \
"`foo[bar].nil? || foo[bar].empty?`."
it_behaves_like "offense", "foo(bar).nil? || foo(bar).empty?",
"foo(bar).blank?",
"Homebrew/Blank: Use `foo(bar).blank?` instead of " \
"`foo(bar).nil? || foo(bar).empty?`."
end
end
Loading

0 comments on commit 9c6dbaa

Please sign in to comment.