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

issyl0 · 2023-03-04T22:54:27Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Fixes rubocop: disallow protocol in cask url verified stanza #14717.
Apparently the "verified" parameter in the URL (present when a Cask's download URL is not the same as its homepage) shouldn't have the protocol (https, http) at the front.
Removing this has happened manually in the past, so here's an autocorrecting RuboCop for it.

It works!

/home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/youdaonote.rb:7:19: C: [Correctable] Cask/Url: Verified URL parameter value should not start with https:// or http://.
      verified:   "https://artifact.lx.netease.com/download/ynote-electron/"
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6652 files inspected, 1 offense detected, 1 offense autocorrectable

BrewTestBot · 2023-03-04T22:54:41Z

Review period will end on 2023-03-07 at 00:00:00 UTC.

issyl0 · 2023-03-04T22:54:51Z

Library/Homebrew/rubocops/cask/url.rb

+# typed: true
+# frozen_string_literal: true
+
+require "forwardable"


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

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)

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. 😆

issyl0 · 2023-03-04T22:55:16Z

Library/Homebrew/rubocops/cask/url.rb

@@ -0,0 +1,37 @@
+# typed: true


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

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?

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.)

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.

I figured it out!

- Apparently the "verified" parameter in the URL (present when a Cask's download URL is not the same as its homepage) shouldn't have the protocol (`https`, `http`) at the front. - Removing this has happened manually in the past, so here's an autocorrecting RuboCop for it.

- The `on_url_stanza` method is now used in two cops, `Url` and `UrlLegacyCommaSeparators`. Make the latter inherit from the former to make Sorbet happy. - The style and typecheck checks now pass fine.

- I suspect these were copy-pasted from other cops, like I did in Homebrew#14886 (comment). - The "forwardable" require is unnecesary if the cop doesn't `extend Forwardable` and use `def_delegator`. - The "uri" require is unnecessary if the cop doesn't call `URI` methods.

- According to Homebrew/brew#14717 it's wrong to have the protocol in the "verified" part of a Cask URL. So there's now an autocorrecting RuboCop for it at Homebrew/brew#14886. This fixes the singular offense.

* Update youdaonote from 7.2.3 to 7.2.4 * youdaonote: Remove protocol from verified stanza - According to Homebrew/brew#14717 it's wrong to have the protocol in the "verified" part of a Cask URL. So there's now an autocorrecting RuboCop for it at Homebrew/brew#14886. This fixes the singular offense. --------- Co-authored-by: Issy Long <me@issyl0.co.uk>

BrewTestBot · 2023-03-06T12:22:05Z

Review period skipped due to critical label.

MikeMcQuaid · 2023-03-06T12:31:21Z

Thanks again @issyl0!

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 4, 2023

issyl0 commented Mar 4, 2023

View reviewed changes

issyl0 force-pushed the rubocop-cask-url-verified-disallow-protocol branch from d2bc47c to fb81024 Compare March 4, 2023 22:58

issyl0 force-pushed the rubocop-cask-url-verified-disallow-protocol branch from fb81024 to 79db987 Compare March 4, 2023 23:00

issyl0 added 4 commits March 5, 2023 16:33

rubocops/cask: Appease Sorbet with the correct inheritance

f4a0bc7

- The `on_url_stanza` method is now used in two cops, `Url` and `UrlLegacyCommaSeparators`. Make the latter inherit from the former to make Sorbet happy. - The style and typecheck checks now pass fine.

rubocops/cask/url: Remove unnecessary require "forwardable"

4791224

rubocops/cask/url: Fix test expectations; regexp for starts_with?

4e06756

test/rubocops/cask/url: Ensure that autocorrection works

d995afc

issyl0 mentioned this pull request Mar 5, 2023

rubocops/cask: Clean up unnecessary requires #14891

Merged

7 tasks

issyl0 marked this pull request as ready for review March 5, 2023 17:26

issyl0 mentioned this pull request Mar 5, 2023

youdaonote: Remove protocol from verified stanza Homebrew/homebrew-cask#142324

Closed

9 tasks

issyl0 enabled auto-merge March 6, 2023 08:55

MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Mar 6, 2023

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 6, 2023

BrewTestBot approved these changes Mar 6, 2023

View reviewed changes

issyl0 merged commit 5a68566 into Homebrew:master Mar 6, 2023

issyl0 deleted the rubocop-cask-url-verified-disallow-protocol branch March 6, 2023 12:22

MikeMcQuaid approved these changes Mar 6, 2023

View reviewed changes

github-actions bot added the outdated PR was locked due to age label Apr 6, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

issyl0 commented Mar 4, 2023 •

edited

BrewTestBot commented Mar 4, 2023

issyl0 Mar 4, 2023

dduugg Mar 5, 2023

issyl0 Mar 5, 2023

issyl0 Mar 4, 2023

issyl0 Mar 4, 2023 •

edited

dduugg Mar 5, 2023

issyl0 Mar 5, 2023

issyl0 Mar 5, 2023

BrewTestBot commented Mar 6, 2023

MikeMcQuaid commented Mar 6, 2023

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

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

Conversation

issyl0 commented Mar 4, 2023 • edited

BrewTestBot commented Mar 4, 2023

issyl0 Mar 4, 2023

Choose a reason for hiding this comment

dduugg Mar 5, 2023

Choose a reason for hiding this comment

issyl0 Mar 5, 2023

Choose a reason for hiding this comment

issyl0 Mar 4, 2023

Choose a reason for hiding this comment

issyl0 Mar 4, 2023 • edited

Choose a reason for hiding this comment

dduugg Mar 5, 2023

Choose a reason for hiding this comment

issyl0 Mar 5, 2023

Choose a reason for hiding this comment

issyl0 Mar 5, 2023

Choose a reason for hiding this comment

BrewTestBot commented Mar 6, 2023

MikeMcQuaid commented Mar 6, 2023

issyl0 commented Mar 4, 2023 •

edited

issyl0 Mar 4, 2023 •

edited