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

rubocops/cask: Disallow protocol in cask URL verified stanza #14886

Merged
Merged
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
2 changes: 1 addition & 1 deletion Library/Homebrew/rubocops/cask/mixin/on_url_stanza.rbi
@@ -1,5 +1,5 @@
# typed: strict

module RuboCop::Cop::Cask::OnUrlStanza
requires_ancestor { RuboCop::Cop::Cask::UrlLegacyCommaSeparators }
requires_ancestor { RuboCop::Cop::Cask::Url }
end
46 changes: 46 additions & 0 deletions Library/Homebrew/rubocops/cask/url.rb
@@ -0,0 +1,46 @@
# typed: true
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorbet doesn't run successfully. Need to read up on https://sorbet.org/docs/requires-ancestor.

Copy link
Member Author

@issyl0 issyl0 Mar 4, 2023

Choose a reason for hiding this comment

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

Reading https://blog.appsignal.com/2021/01/13/using-mixins-and-modules-in-your-ruby-on-rails-application.html for a general overview of mixins, and https://sorbet.org/docs/requires-ancestor, I do not understand what it wants me to do. As the person who introduced this requires_ancestor thing, @dduugg can you shed any light (either here or in Slack)? Have I taken a wrong approach here somewhere?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

module RuboCop::Cop::Cask::OnUrlStanza
  requires_ancestor { RuboCop::Cop::Cask::UrlLegacyCommaSeparators }
end

This code means that any module that includes RuboCop::Cop::Cask::OnUrlStanza must subclass RuboCop::Cop::Cask::UrlLegacyCommaSeparators. This makes the methods defined in the latter available in the former. (In this specific case, OnUrlStanza makes use of on_url_stanza, the lone method defined in UrlLegacyCommaSeparators.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, so if we now have RuboCop::Cop::Cask::Url and RuboCop::Cop::Cask::UrlLegacyCommaSeparators cops that have the on_url_stanza method , which one should inherit from which? They're separate cops.

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 figured it out!

# frozen_string_literal: true

module RuboCop
module Cop
module Cask
# This cop checks that a cask's `url` stanza is formatted correctly.
#
# @example
# # bad
# url "https://example.com/foo.dmg",
# verified: "https://example.com"
#
#
# # good
# url "https://example.com/foo.dmg",
# verified: "example.com"
#
class Url < Base
extend AutoCorrector
extend Forwardable
include OnUrlStanza

def on_url_stanza(stanza)
return if stanza.stanza_node.block_type?

hash_node = stanza.stanza_node.last_argument
return unless hash_node.hash_type?

hash_node.each_pair do |key_node, value_node|
next unless key_node.source == "verified"
next unless value_node.str_type?
next unless value_node.source.start_with?(%r{^"https?://})

add_offense(
value_node.source_range,
message: "Verified URL parameter value should not start with https:// or http://.",
) do |corrector|
corrector.replace(value_node.source_range, value_node.source.gsub(%r{^"https?://}, "\""))
end
end
end
end
end
end
end
Expand Up @@ -8,7 +8,7 @@ module RuboCop
module Cop
module Cask
# This cop checks for version.before_comma and version.after_comma
class UrlLegacyCommaSeparators < Base
class UrlLegacyCommaSeparators < Url
include OnUrlStanza
extend AutoCorrector

Expand Down
1 change: 1 addition & 0 deletions Library/Homebrew/rubocops/rubocop-cask.rb
Expand Up @@ -18,5 +18,6 @@
require_relative "cask/on_system_conditionals"
require_relative "cask/stanza_order"
require_relative "cask/stanza_grouping"
require_relative "cask/url"
require_relative "cask/url_legacy_comma_separators"
require_relative "cask/variables"
57 changes: 57 additions & 0 deletions Library/Homebrew/test/rubocops/cask/url_spec.rb
@@ -0,0 +1,57 @@
# typed: false
# frozen_string_literal: true

require "rubocops/rubocop-cask"
require "test/rubocops/cask/shared_examples/cask_cop"

describe RuboCop::Cop::Cask::Url do
include CaskCop

subject(:cop) { described_class.new }

context "when url 'verified' value does not start with a protocol" do
let(:source) do
<<~CASK
cask "foo" do
url "https://example.com/download/foo-v1.2.0.dmg",
verified: "example.com"
end
CASK
end

include_examples "does not report any offenses"
end

context "when url 'verified' value starts with a protocol" do
let(:source) do
<<~CASK
cask "foo" do
url "https://example.com/download/foo-v1.2.0.dmg",
verified: "https://example.com"
end
CASK
end

let(:expected_offenses) do
[{
message: "Verified URL parameter value should not start with https:// or http://.",
severity: :convention,
line: 3,
column: 14,
source: "\"https://example.com\"",
}]
end

let(:correct_source) do
<<~CASK
cask "foo" do
url "https://example.com/download/foo-v1.2.0.dmg",
verified: "example.com"
end
CASK
end

include_examples "reports offenses"
include_examples "autocorrects source"
end
end