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

cask/audit: allow dots in version and check unversioned cask #16882

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 38 additions & 12 deletions Library/Homebrew/cask/audit.rb
Expand Up @@ -410,17 +410,48 @@
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? "@-"
unversioned_token, _, version_designation = cask.token.rpartition("@")

Check warning on line 417 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L417

Added line #L417 was not covered by tests
if unversioned_token.empty?
match_data = /-(?<designation>alpha|beta|rc|release-candidate)$/.match(cask.token)
if match_data && cask.tap&.official? && cask.tap != "homebrew/cask-versions"
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
if match_data && cask.tap&.official? && cask.tap != "homebrew/cask-versions"
if match_data && cask.tap&.official? && cask.tap.name != "homebrew/cask-versions"

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if we should only apply to homebrew/cask given audit failures in fonts. From original location of check (which I'll probably move code back to later), I'm guessing we either don't apply that audit or there were no new fonts with -beta|alpha.

As I recall, we have automation there that picks those names?

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely fix the automation, but maybe this should only apply to brew audit --new for now?

add_error "cask token should use @ before version designation '#{match_data[:designation]}'"
end
unversioned_token = cask.token
version_designation = ""

Check warning on line 424 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L423-L424

Added lines #L423 - L424 were not covered by tests
end

add_error "unversioned cask token @ should be replaced by -at-" if unversioned_token.include? "@"
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
add_error "unversioned cask token @ should be replaced by -at-" if unversioned_token.include? "@"
add_error "cask token should use '-at-' instead of '@' in the unversioned part" if unversioned_token.include? "@"

Copy link
Member Author

@cho-m cho-m Mar 12, 2024

Choose a reason for hiding this comment

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

Thanks. I just realized that "unversioned cask" was poor phrasing given its existing usage for unversioned URLs (bump-unversioned-casks).


if unversioned_token.match?(/[^a-z0-9-]/)
add_error "unversioned cask token should only contain lowercase alphanumeric characters and hyphens"
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
add_error "unversioned cask token should only contain lowercase alphanumeric characters and hyphens"
add_error "cask token should only contain lowercase alphanumeric characters and hyphens"

end

return if version_designation.empty?

add_error "unversioned cask token should not have trailing hyphens" if unversioned_token.end_with?("-")
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
add_error "unversioned cask token should not have trailing hyphens" if unversioned_token.end_with?("-")
add_error "cask token should not have trailing hyphens" if unversioned_token.end_with?("-")


if version_designation.match?(/[^.a-z0-9-]/)
add_error "cask token version designation should only contain " \
"lowercase alphanumeric characters, hyphens and '.'"
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
"lowercase alphanumeric characters, hyphens and '.'"
"lowercase alphanumeric characters, hyphens and dots"

end

if version_designation.start_with?("-", ".") || version_designation.end_with?(".")
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
if version_designation.start_with?("-", ".") || version_designation.end_with?(".")
if version_designation.start_with?("-", ".") || version_designation.end_with?("-", ".")

add_error "cask token version designation should not have leading or trailing hyphens and/or '.'"
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
add_error "cask token version designation should not have leading or trailing hyphens and/or '.'"
add_error "cask token version designation should not have leading or trailing hyphens and/or dots"

end

return unless cask.tap&.official?
return if cask.tap&.audit_exception(:versioned_no_unversioned_allowlist, cask.token)

unversioned_full_token = "#{cask.tap}/#{unversioned_token}"

Check warning on line 449 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L449

Added line #L449 was not covered by tests
begin
CaskLoader.load(unversioned_full_token)
rescue CaskUnavailableError
add_error "versioned cask but no #{unversioned_full_token} cask exists"

Check warning on line 453 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L451-L453

Added lines #L451 - L453 were not covered by tests
end
end

sig { void }
Expand All @@ -431,11 +462,6 @@

add_error "cask token contains .app" if token.end_with? ".app"

match_data = /-(?<designation>alpha|beta|rc|release-candidate)$/.match(cask.token)
if match_data && cask.tap&.official? && cask.tap != "homebrew/cask-versions"
add_error "cask token contains version designation '#{match_data[:designation]}'"
end

add_error("cask token mentions launcher", strict_only: true) if token.end_with? "launcher"

add_error("cask token mentions desktop", strict_only: true) if token.end_with? "desktop"
Expand Down
52 changes: 30 additions & 22 deletions Library/Homebrew/test/cask/audit_spec.rb
Expand Up @@ -220,8 +220,8 @@ def tmp_cask(name, text)
end
end

context "when cask token is @-versioned with number" do
let(:cask_token) { "app@10" }
context "when cask token is @-versioned with major-minor version number" do
let(:cask_token) { "app@10.2" }

it "does not fail" do
expect(run).to pass
Expand All @@ -240,23 +240,23 @@ 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(/unversioned cask token @ should be replaced by -at-/)
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(/unversioned cask token should not have trailing hyphens/)
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(%r{should not have leading or trailing hyphens and/or '.'})
end
end

Expand All @@ -280,7 +280,15 @@ def tmp_cask(name, text)
let(:cask_token) { "app(stuff)" }

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

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

it "fails" do
expect(run).to error_with(/alphanumeric characters, hyphens and '.'/)
end
end

Expand All @@ -307,6 +315,22 @@ def tmp_cask(name, text)
expect(run).to error_with(/should not have leading or trailing hyphens/)
end
end

context "when cask token contains version designation without @" do
let(:cask_token) { "token-beta" }

it "fails if the cask is from an official tap" do
allow(cask).to receive(:tap).and_return(CoreCaskTap.instance)

expect(run).to error_with(/should use @ before version designation 'beta'/)
end

it "does not fail if the cask is from the `cask-versions` tap" do
allow(cask).to receive(:tap).and_return(Tap.fetch("homebrew/cask-versions"))

expect(run).to pass
end
end
end

describe "token bad words" do
Expand Down Expand Up @@ -335,22 +359,6 @@ def tmp_cask(name, text)
end
end

context "when cask token contains version designation" do
let(:cask_token) { "token-beta" }

it "fails if the cask is from an official tap" do
allow(cask).to receive(:tap).and_return(CoreCaskTap.instance)

expect(run).to error_with(/token contains version designation/)
end

it "does not fail if the cask is from the `cask-versions` tap" do
allow(cask).to receive(:tap).and_return(Tap.fetch("homebrew/cask-versions"))

expect(run).to pass
end
end

context "when cask token contains launcher" do
let(:cask_token) { "token-launcher" }

Expand Down