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

Share code between cask token and formula name audits. #17562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
31 changes: 10 additions & 21 deletions Library/Homebrew/cask/audit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require "livecheck/livecheck"
require "source_location"
require "system_command"
require "token_auditor"
require "utils/curl"
require "utils/git"
require "utils/shared_audits"
Expand Down Expand Up @@ -391,6 +392,15 @@ def audit_languages
end
end

sig { void }
def audit_token
token_auditor = Homebrew::TokenAuditor.new(cask.token)
return if (errors = token_auditor.errors).empty?

add_error "Cask token '#{cask.token}' must not contain #{errors.to_sentence(two_words_connector: " or ",
last_word_connector: " or ")}."
end

sig { void }
def audit_token_conflicts
return unless token_conflicts?
Expand All @@ -405,27 +415,6 @@ def audit_token_conflicts
end
end

sig { void }
def audit_token_valid
add_error "cask token contains non-ascii characters" unless cask.token.ascii_only?
add_error "cask token + should be replaced by -plus-" if cask.token.include? "+"
add_error "cask token whitespace should be replaced by hyphens" if cask.token.include? " "
add_error "cask token underscores should be replaced by hyphens" if cask.token.include? "_"
add_error "cask token should not contain double hyphens" if cask.token.include? "--"

if cask.token.match?(/[^@a-z0-9-]/)
add_error "cask token should only contain lowercase alphanumeric characters, hyphens and @"
end

if cask.token.start_with?("-", "@") || cask.token.end_with?("-", "@")
add_error "cask token should not have leading or trailing hyphens and/or @"
end

add_error "cask token @ unrelated to versioning should be replaced by -at-" if cask.token.count("@") > 1
add_error "cask token should not contain a hyphen followed by @" if cask.token.include? "-@"
add_error "cask token should not contain @ followed by a hyphen" if cask.token.include? "@-"
end

sig { void }
def audit_token_bad_words
return unless new_cask?
Expand Down
9 changes: 7 additions & 2 deletions Library/Homebrew/formula_auditor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

require "deprecate_disable"
require "formula_versions"
require "token_auditor"
require "resource_auditor"
require "utils/shared_audits"

Expand Down Expand Up @@ -160,10 +161,14 @@ def audit_synced_versions_formulae
end
end

def audit_formula_name
def audit_name
name = formula.name

problem "Formula name '#{name}' must not contain uppercase letters." if name != name.downcase
token_auditor = Homebrew::TokenAuditor.new(name)
unless (errors = token_auditor.errors).empty?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
unless (errors = token_auditor.errors).empty?
if (errors = token_auditor.errors).any?

or

Suggested change
unless (errors = token_auditor.errors).empty?
if (errors = token_auditor.presence)

problem "Formula name '#{name}' must not contain #{errors.to_sentence(two_words_connector: " or ",
last_word_connector: " or ")}."
end

return unless @strict
return unless @core_tap
Expand Down
44 changes: 10 additions & 34 deletions Library/Homebrew/test/cask/audit_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ def tmp_cask(name, text)

describe "token validation" do
let(:strict) { true }
let(:only) { ["token_valid"] }
let(:only) { ["token"] }
let(:cask) do
tmp_cask cask_token.to_s, <<~RUBY
cask '#{cask_token}' do
Expand All @@ -200,23 +200,15 @@ def tmp_cask(name, text)
let(:cask_token) { "Upper-Case" }

it "fails" do
expect(run).to error_with(/lowercase/)
expect(run).to error_with(/not contain uppercase/)
end
end

context "when cask token is not ascii" do
let(:cask_token) { "ascii⌘" }

it "fails" do
expect(run).to error_with(/contains non-ascii characters/)
end
end

context "when cask token has +" do
let(:cask_token) { "app++" }

it "fails" do
expect(run).to error_with(/\+ should be replaced by -plus-/)
expect(run).to error_with(/not contain non-ASCII characters/)
end
end

Expand All @@ -240,71 +232,55 @@ def tmp_cask(name, text)
let(:cask_token) { "app@stuff@beta" }

it "fails" do
expect(run).to error_with(/@ unrelated to versioning should be replaced by -at-/)
expect(run).to error_with(/not contain multiple @ symbols/)
end
end

context "when cask token has a hyphen followed by @" do
let(:cask_token) { "app-@beta" }

it "fails" do
expect(run).to error_with(/should not contain a hyphen followed by @/)
expect(run).to error_with(/not contain a hyphen followed by an @/)
end
end

context "when cask token has @ followed by a hyphen" do
let(:cask_token) { "app@-beta" }

it "fails" do
expect(run).to error_with(/should not contain @ followed by a hyphen/)
expect(run).to error_with(/not contain an @ followed by a hyphen/)
end
end

context "when cask token has whitespace" do
let(:cask_token) { "app stuff" }

it "fails" do
expect(run).to error_with(/whitespace should be replaced by hyphens/)
end
end

context "when cask token has underscores" do
let(:cask_token) { "app_stuff" }

it "fails" do
expect(run).to error_with(/underscores should be replaced by hyphens/)
end
end

context "when cask token has non-alphanumeric characters" do
let(:cask_token) { "app(stuff)" }

it "fails" do
expect(run).to error_with(/alphanumeric characters, hyphens and @/)
expect(run).to error_with(/not contain whitespace/)
end
end

context "when cask token has double hyphens" do
let(:cask_token) { "app--stuff" }

it "fails" do
expect(run).to error_with(/should not contain double hyphens/)
expect(run).to error_with(/not contain double hyphens/)
end
end

context "when cask token has leading hyphens" do
let(:cask_token) { "-app" }

it "fails" do
expect(run).to error_with(/should not have leading or trailing hyphens/)
expect(run).to error_with(/not contain a leading hyphen/)
end
end

context "when cask token has trailing hyphens" do
let(:cask_token) { "app-" }

it "fails" do
expect(run).to error_with(/should not have leading or trailing hyphens/)
expect(run).to error_with(/not contain a trailing hyphen/)
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions Library/Homebrew/test/formula_auditor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ class Foo < Formula
end
end

describe "#audit_formula_name" do
describe "#audit_name" do
specify "no issue" do
fa = formula_auditor "foo", <<~RUBY, core_tap: true, strict: true
class Foo < Formula
Expand All @@ -485,7 +485,7 @@ class Foo < Formula
end
RUBY

fa.audit_formula_name
fa.audit_name
expect(fa.problems).to be_empty
end

Expand All @@ -497,7 +497,7 @@ class Foo < Formula
end
RUBY

fa.audit_formula_name
fa.audit_name
expect(fa.problems.first[:message]).to match "must not contain uppercase letters"
end
end
Expand Down
40 changes: 40 additions & 0 deletions Library/Homebrew/token_auditor.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# typed: true
# frozen_string_literal: true

module Homebrew
class TokenAuditor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class TokenAuditor
class NameAuditor

Thoughts? Token is cask-specific so I think maybe "name" is clearer in this case. NBD though either way

Copy link
Member Author

Choose a reason for hiding this comment

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

name has a different meaning for casks.

Copy link
Member

@Rylan12 Rylan12 Jun 27, 2024

Choose a reason for hiding this comment

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

True, but also token has no meaning for formulae. When I'm speaking about what to call a formula or cask, I use the word "name" and mean Formula#name and Cask#token.

I'll leave it up to other people for their thoughts

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class TokenAuditor
class FormulaNameCaskTokenAuditor

or something.

I agree TokenAuditor doesn't make sense if it can operate on Formula because they don't have tokens.

sig { returns(String) }
attr_reader :token

sig { params(token: String).void }
def initialize(token)
@token = token
end

sig { returns(T::Array[String]) }
def errors
errors = []

errors << "uppercase letters" if token.match?(/[A-Z]/)
errors << "whitespace" if token.match?(/\s/)
errors << "non-ASCII characters" unless token.ascii_only?
errors << "double hyphens" if token.include?("--")

# A bunch of formulae contain these:
# errors << "underscores" if token.include?("_")
# errors << "plus symbols" if token.include?("+")
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

Maybe accept the formula or cask itself in the initializer and then check these only if it's a cask?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given we allow _ and + for formula names, I don't really see a good reason to disallow it for casks.

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly at the moment, but the comments should be removed if this is the plan

Copy link
Member Author

@reitermarkus reitermarkus Jun 27, 2024

Choose a reason for hiding this comment

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

Yes, I'll remove the comments before merging. We should check if casks should be renamed/aliased (plus -> +).


errors << "a leading @" if token.start_with?("@")
errors << "a trailing @" if token.end_with?("@")
errors << "a leading hyphen" if token.start_with?("-")
errors << "a trailing hyphen" if token.end_with?("-")

errors << "multiple @ symbols" if token.count("@") > 1

errors << "a hyphen followed by an @" if token.include? "-@"
errors << "an @ followed by a hyphen" if token.include? "@-"

errors
end
end
end