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 1 commit
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
48 changes: 48 additions & 0 deletions Library/Homebrew/rubocops/cask/url.rb
@@ -0,0 +1,48 @@
# 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

require "forwardable"
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what this does, I copied and pasted from other cops. 😳

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think it's meaningless without any use of def_delegator(s): https://ruby-doc.org/stdlib-2.5.1/libdoc/forwardable/rdoc/Forwardable.html (which would make a nice cop by itself)

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 thought so, but it's in a few cops that don't make use of def_delegators either. I guess that's a mistake. One RuboCop at a time for me, though. 😆


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?("\"https:/", "\"http:/")

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
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"
47 changes: 47 additions & 0 deletions Library/Homebrew/test/rubocops/cask/url_spec.rb
@@ -0,0 +1,47 @@
# 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: "verified: \"https://example.com\"",
}]
end

include_examples "reports offenses"
end
end