Type `livecheck.rb`. #15279

reitermarkus · 2023-04-20T23:23:52Z

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?

MikeMcQuaid

Sorry, I don't think having this in livecheck is the right approach. As mentioned in Slack, audit.rb already provides throttling for formulae: we should use the same approach for casks.

Library/Homebrew/livecheck.rb

samford

Sorry, I don't think having this in livecheck is the right approach. As mentioned in Slack, audit.rb already provides throttling for formulae: we should use the same approach for casks.

I agree with Mike. I unfortunately wasn't explicit in past discussions but when I mentioned handling throttling outside of livecheck, I was thinking that it would be in line with the setup that we have for formulae (e.g., handled in an audit_exceptions/throttled_casks.json file in cask taps).

reitermarkus · 2023-04-21T14:18:23Z

audit.rb already provides throttling for formulae

This is based on “version is a multiple of n”, which definitely will not work for most cask versions.

Also, having this in audit is the wrong approach in my opinion. It handles the symptom (open version bump PRs), not the source of the problem (opening them in the first place). At the point where we run CI for a PR, a failing audit is more maintenance work than just approving the version bump. So it should be handled in bump-* commands. (I am aware bump-* commands run audit by default, but throttling should work even with --no-audit.)

MikeMcQuaid · 2023-04-24T08:27:58Z

Happy to merge one type-checking is 💚, thanks @reitermarkus!

This is based on “version is a multiple of n”, which definitely will not work for most cask versions.

@reitermarkus can you elaborate on why you don't think this will work and what you were/are proposing instead?

(I am aware bump-* commands run audit by default, but throttling should work even with --no-audit.)

I don't agree that this is necessary, honestly. This doesn't seem different to another audit and I'd like to see evidence that these audits are being ignored in bumps before we deviate approach here.

In general: I think casks and formulae for both user and contributors should be as similar as possible. Where they deviate it adds friction, bugs and confusing for everyone. Sometimes this is necessary, many times it is not.

SMillerDev · 2023-04-24T09:07:29Z

Ignoring audits in bumps was suggested here: Homebrew/homebrew-cask#145518

dduugg · 2023-04-25T18:57:23Z

Library/Homebrew/livecheck.rb

@@ -13,12 +13,9 @@
 # [`brew livecheck` documentation](https://docs.brew.sh/Brew-Livecheck).
 class Livecheck
  extend Forwardable
+  extend T::Sig


This can (and needs to be) removed due to #15306

reitermarkus · 2023-04-26T12:17:19Z

@samford and @dduugg, what do we want to do here regarding TypeErrors? Remove the explicit errors or just simplify the tests to not check the error message?

dduugg · 2023-04-26T16:41:04Z

@samford and @dduugg, what do we want to do here regarding TypeErrors? Remove the explicit errors or just simplify the tests to not check the error message?

Testing the specific content of error messages is fragile IME (for instance, it could also break on a sorbet update).

If you waned to be thorough, one suggestion could be a test helper that expects a TypeError with the expected and actual types, and converts that to a regexp to match against the actual TypeError message. WDYT?

reitermarkus · 2023-04-26T16:53:28Z

I think just testing for any TypeError is fine.

My question was more a decision between these options:

Remove the explicit exceptions and the test.
Leave the explicit exception and adjust the test.
Ensure the test runs without sorbet-runtime. Not sure if that's possible, but it's also not preferable.

dduugg · 2023-04-26T17:05:20Z

I think just testing for any TypeError is fine.

My question was more a decision between these options:

Remove the explicit exceptions and the test.

Leave the explicit exception and adjust the test.

Ensure the test runs without sorbet-runtime. Not sure if that's possible, but it's also not preferable.

Ah, ok, I think I better understand now. Yeah, testing that a sig'd method raises a TypeError with the wrong type would generally not be useful. (We do have the wrinkle though that folks generally have runtime type checking disabled, but also that tests force runtime typechecking 🤕. I guess it would come down to the specific code and how likely it is for an end user to be able to force the wrong types in…)

samford

What do we want to do here regarding TypeErrors? Remove the explicit errors or just simplify the tests to not check the error message?

Since type checking doesn't come into play when brew livecheck is run, we still need the manual exceptions in the DSL methods. We can't cover those lines with tests after the Livecheck DSL is typed (since Sorbet raises a TypeError before we can reach the manual exception), so keeping those test cases around wouldn't make a functional difference. We would basically be testing Sorbet at that point, which we don't want to do.

Unless we can disable Sorbet's type checking for these specific test cases, I would say: leave the manual exceptions in the Livecheck DSL methods and we can just remove the related test cases in livecheck_spec.rb. There's some related Sorbet runtime documentation that seemingly explains how to control the runtime for specific type signatures but I'm not sure if any of it is feasible in relation to how we do things with brew typecheck, brew tests, etc. I haven't looked into it but I figured I would mention it, at least.

The other test failures I'm seeing relate to the signature for package_or_resource. We should arguably be using Cask::Cask (since that's what we're working with in brew livecheck) but that leads to a bunch of type errors based on how we're creating test cask objects, so we'll need to find a way of creating the test casks as Cask::Cask objects.

I'm also seeing failures in relation to livecheckable_f in livecheck_spec.rb (e.g., Parameter 'package_or_resource': Expected type T.any(Cask::Cask, Resource, T.class_of(Formula)), got type #<Class:0x0000000116a2d4d0> with value #<Formula formula_name (sta...-core/Formula/formula_name.rb>) but I'm not immediately sure what the issue is there.

MikeMcQuaid · 2023-04-27T07:33:43Z

Since type checking doesn't come into play when brew livecheck is run, we still need the manual exceptions in the DSL methods.

It could very easily. We could start setting HOMEBREW_SORBET_RUNTIME when running brew livecheck/all developer commands/anything when HOMEBREW_DEVELOPER is set. This seems like a much more sensible route forward than duplicating the work Sorbet is doing.

reitermarkus · 2023-04-27T16:02:03Z

It could very easily.

Yes, I agree. At the very least audit should probably run with sorbet-runtime.

samford · 2023-04-28T07:32:57Z

It could very easily. We could start setting HOMEBREW_SORBET_RUNTIME when running brew livecheck/all developer commands/anything when HOMEBREW_DEVELOPER is set. This seems like a much more sensible route forward than duplicating the work Sorbet is doing.

Makes sense to me. I haven't ever run livecheck with HOMEBREW_SORBET_RUNTIME but I'll have to test it out sometime. If our existing manual type checking code has been doing its job, hopefully the Sorbet runtime won't surface any new errors that we weren't already handling.

MikeMcQuaid · 2023-04-28T08:07:06Z

Yes, I agree. At the very least audit should probably run with sorbet-runtime.
I haven't ever run livecheck with HOMEBREW_SORBET_RUNTIME but I'll have to test it out sometime.

I've opened a PR to bit the bullet and enable this for all Homebrew's developer commands and everyone with HOMEBREW_DEVELOPER set: #15326

MikeMcQuaid · 2023-04-28T08:07:43Z

(assuming that PR gets merged: I think, yes, we can strip all TypeError checks out of all dev-cmd)

dduugg · 2023-05-02T20:56:00Z

Library/Homebrew/livecheck.rb

@@ -37,7 +35,8 @@ def initialize(package_or_resource)
  #
  # @param cask_name [String] name of cask to inherit livecheck info from
  # @return [String, nil]


what do you think of removing the YARD type annotations? (yard-sorbet is just going to use the sig types anyway)

The YARD types are redundant (as mentioned) but the parameter descriptions are still useful for documentation purposes. If we can keep the descriptions but omit the types, that makes sense to me (i.e., we wouldn't have to waste time keeping the YARD types in line with the Sorbet method signatures).

Agreed that it'd be nice to try and drop these types.

It'd be nice if we could just swap out all the YARD types for sorbet sigs. I filed AaronC81/sord#167 since that's the most similar project, I should have some bandwidth to take a stab at it in the summer.

Ideally, docs would be correctly generated from

sig { params( # name of cask to inherit livecheck info from cask_name: String ).returns(T.nilable(String)) }

dduugg

The requested YARD changes look good. I have a couple questions, but they aren't blocking (and apologies if i missed any earlier discussion).

Also, I'd rather we use T.absurd for exhuastiveness checking over the approach we're using here (and elsewhere), but that would require us to enable runtime errors for that specific method (which isn't difficult, just something we've consciously avoided for all sorbet runtime checks).

Library/Homebrew/livecheck.rb

samford · 2023-05-05T05:07:28Z

I ran out of time today but I'll test and review this again in the next day or two (hopefully Friday evening, EDT).

MikeMcQuaid

One tweak needed and good to merge 👍🏻. Thanks @reitermarkus!

Library/Homebrew/livecheck.rb

reitermarkus requested review from samford and razvanazamfirei April 20, 2023 23:23

reitermarkus force-pushed the livecheck-throttle branch from e7f467e to db5bafa Compare April 20, 2023 23:27

MikeMcQuaid requested changes Apr 21, 2023

View reviewed changes

Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved

samford reviewed Apr 21, 2023

View reviewed changes

reitermarkus force-pushed the livecheck-throttle branch from db5bafa to 8c9a779 Compare April 21, 2023 21:44

reitermarkus changed the title ~~Add throttle stanza for livecheck.~~ Type livecheck.rb. Apr 21, 2023

MikeMcQuaid approved these changes Apr 24, 2023

View reviewed changes

dduugg reviewed Apr 25, 2023

View reviewed changes

samford reviewed Apr 27, 2023

View reviewed changes

dduugg reviewed May 2, 2023

View reviewed changes

reitermarkus force-pushed the livecheck-throttle branch 3 times, most recently from 5e6a11e to eb91a87 Compare May 4, 2023 18:25

reitermarkus requested review from MikeMcQuaid, samford and dduugg May 4, 2023 19:17

dduugg approved these changes May 4, 2023

View reviewed changes

Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved

Library/Homebrew/livecheck.rb Show resolved Hide resolved

MikeMcQuaid approved these changes May 5, 2023

View reviewed changes

Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved

Library/Homebrew/livecheck.rb Show resolved Hide resolved

reitermarkus force-pushed the livecheck-throttle branch from eb91a87 to 2eb41b1 Compare May 6, 2023 01:26

Type livecheck.rb.

dfc9d94

reitermarkus force-pushed the livecheck-throttle branch from 2eb41b1 to dfc9d94 Compare May 6, 2023 01:27

reitermarkus merged commit 477a26c into Homebrew:master May 6, 2023
24 checks passed

reitermarkus deleted the livecheck-throttle branch May 6, 2023 21:20

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type `livecheck.rb`. #15279

Type `livecheck.rb`. #15279

reitermarkus commented Apr 20, 2023 •

edited

Loading

MikeMcQuaid left a comment

samford left a comment •

edited

Loading

reitermarkus commented Apr 21, 2023

MikeMcQuaid commented Apr 24, 2023

SMillerDev commented Apr 24, 2023

dduugg Apr 25, 2023

reitermarkus commented Apr 26, 2023

dduugg commented Apr 26, 2023

reitermarkus commented Apr 26, 2023 •

edited

Loading

dduugg commented Apr 26, 2023

samford left a comment

MikeMcQuaid commented Apr 27, 2023

reitermarkus commented Apr 27, 2023

samford commented Apr 28, 2023

MikeMcQuaid commented Apr 28, 2023

MikeMcQuaid commented Apr 28, 2023

dduugg May 2, 2023

samford May 2, 2023

MikeMcQuaid May 3, 2023

dduugg May 3, 2023

reitermarkus May 4, 2023

dduugg left a comment

samford commented May 5, 2023

MikeMcQuaid left a comment

Type livecheck.rb. #15279

Type livecheck.rb. #15279

Conversation

reitermarkus commented Apr 20, 2023 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

samford left a comment • edited Loading

Choose a reason for hiding this comment

reitermarkus commented Apr 21, 2023

MikeMcQuaid commented Apr 24, 2023

SMillerDev commented Apr 24, 2023

dduugg Apr 25, 2023

Choose a reason for hiding this comment

reitermarkus commented Apr 26, 2023

dduugg commented Apr 26, 2023

reitermarkus commented Apr 26, 2023 • edited Loading

dduugg commented Apr 26, 2023

samford left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 27, 2023

reitermarkus commented Apr 27, 2023

samford commented Apr 28, 2023

MikeMcQuaid commented Apr 28, 2023

MikeMcQuaid commented Apr 28, 2023

dduugg May 2, 2023

Choose a reason for hiding this comment

samford May 2, 2023

Choose a reason for hiding this comment

MikeMcQuaid May 3, 2023

Choose a reason for hiding this comment

dduugg May 3, 2023

Choose a reason for hiding this comment

reitermarkus May 4, 2023

Choose a reason for hiding this comment

dduugg left a comment

Choose a reason for hiding this comment

samford commented May 5, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Type `livecheck.rb`. #15279

Type `livecheck.rb`. #15279

reitermarkus commented Apr 20, 2023 •

edited

Loading

samford left a comment •

edited

Loading

reitermarkus commented Apr 26, 2023 •

edited

Loading