Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formula: allow excluding deprecate! reason when disable! exists #18661

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions Library/Homebrew/formula.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4237,12 +4237,10 @@ def pour_bottle?(only_if: nil, &block)
# @see https://docs.brew.sh/Deprecating-Disabling-and-Removing-Formulae
# @see DeprecateDisable::FORMULA_DEPRECATE_DISABLE_REASONS
# @api public
def deprecate!(date:, because:)
def deprecate!(date:, because: nil)
MikeMcQuaid marked this conversation as resolved.
Show resolved Hide resolved
@deprecation_date = Date.parse(date)
return if @deprecation_date > Date.today

@deprecation_reason = because
@deprecated = true
@deprecated = !disabled? && @deprecation_date <= Date.today
@deprecation_reason = because if because
end

# Whether this {Formula} is deprecated (i.e. warns on installation).
Expand Down Expand Up @@ -4288,8 +4286,8 @@ def disable!(date:, because:)
@disable_date = Date.parse(date)

if @disable_date > Date.today
@deprecation_reason = because
@deprecated = true
@deprecation_reason ||= because
@deprecated = true if deprecation_date.nil?
return
end

Expand Down
20 changes: 16 additions & 4 deletions Library/Homebrew/rubocops/deprecate_disable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ class DeprecateDisableReason < FormulaCop
sig { override.params(formula_nodes: FormulaNodes).void }
def audit_formula(formula_nodes)
body_node = formula_nodes.body_node
reason_nodes = T.let([], T::Array[T.any(AST::StrNode, AST::SymbolNode)])

[:deprecate!, :disable!].each do |method|
[:disable!, :deprecate!].each do |method|
node = find_node_method_by_name(body_node, method)

next if node.nil?

reason_found = T.let(false, T::Boolean)
reason(node) do |reason_node|
reason_found = true
reason_nodes << reason_node
next if reason_node.sym_type?

offending_node(reason_node)
Expand All @@ -72,7 +72,7 @@ def audit_formula(formula_nodes)
end
end

next if reason_found
next unless reason_nodes.empty?

case method
when :deprecate!
Expand All @@ -81,6 +81,18 @@ def audit_formula(formula_nodes)
problem 'Add a reason for disabling: `disable! because: "..."`'
end
end

return if reason_nodes.length < 2

disable_reason_node = T.must(reason_nodes.first)
deprecate_reason_node = T.must(reason_nodes.second)
return if disable_reason_node.value != deprecate_reason_node.value

offending_node(deprecate_reason_node.parent)
problem "Remove deprecate reason when disable reason is identical" do |corrector|
range = deprecate_reason_node.parent.source_range
corrector.remove(range_with_surrounding_comma(range_with_surrounding_space(range:, side: :left)))
end
end

def_node_search :reason, <<~EOS
Expand Down
61 changes: 61 additions & 0 deletions Library/Homebrew/test/rubocops/deprecate_disable/reason_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -324,4 +324,65 @@ class Foo < Formula
RUBY
end
end

context "when auditing `deprecate!` and `disable!`" do
it "reports no offense if deprecate `reason` is absent" do
expect_no_offenses(<<~RUBY)
class Foo < Formula
url 'https://brew.sh/foo-1.0.tgz'
disable! date: "2021-08-28", because: :does_not_build
deprecate! date: "2020-08-28"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really relevant for this PR, but are these automatically sorted in this order by another cop? To me it would make more sense to have deprecate! sorted first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is from AST order:

[{ name: :disable!, type: :method_call }],
[{ name: :deprecate!, type: :method_call }],

Could reverse them if we wanted, though would need to make sure about handling. I did try to add code so order doesn't matter.

end
RUBY
end

it "reports offense if disable `reason` is absent`" do
expect_offense(<<~RUBY)
class Foo < Formula
url 'https://brew.sh/foo-1.0.tgz'
disable! date: "2021-08-28"
^^^^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/DeprecateDisableReason: Add a reason for disabling: `disable! because: "..."`
deprecate! date: "2020-08-28", because: :does_not_build
end
RUBY
end

it "reports and corrects an offense if disable and deprecate `reason` are identical symbols" do
expect_offense(<<~RUBY)
class Foo < Formula
url 'https://brew.sh/foo-1.0.tgz'
disable! date: "2021-08-28", because: :does_not_build
deprecate! date: "2020-08-28", because: :does_not_build
^^^^^^^^^^^^^^^^^^^^^^^^ FormulaAudit/DeprecateDisableReason: Remove deprecate reason when disable reason is identical
end
RUBY

expect_correction(<<~RUBY)
class Foo < Formula
url 'https://brew.sh/foo-1.0.tgz'
disable! date: "2021-08-28", because: :does_not_build
deprecate! date: "2020-08-28"
end
RUBY
end

it "reports and corrects an offense if disable and deprecate `reason` are identical strings" do
expect_offense(<<~RUBY)
class Foo < Formula
url 'https://brew.sh/foo-1.0.tgz'
disable! date: "2021-08-28", because: "is broken"
deprecate! date: "2020-08-28", because: "is broken"
^^^^^^^^^^^^^^^^^^^^ FormulaAudit/DeprecateDisableReason: Remove deprecate reason when disable reason is identical
end
RUBY

expect_correction(<<~RUBY)
class Foo < Formula
url 'https://brew.sh/foo-1.0.tgz'
disable! date: "2021-08-28", because: "is broken"
deprecate! date: "2020-08-28"
end
RUBY
end
end
end
Loading