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

Curl: Use typed: strict #19077

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Curl: Use typed: strict #19077

wants to merge 2 commits into from

Conversation

samford
Copy link
Member

@samford samford commented Jan 11, 2025

  • 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?

This upgrades utils/curl.rb to typed: strict, which requires a number of changes to pass brew typecheck. The most straightforward are adding type signatures to methods, adding type annotations (e.g., T.let) to variables that need them, and ensuring that methods always use the expected return type.

I had to refactor areas where we call a Utils::Curl method and use array destructuring on a SystemCommand::Result return value (e.g., output, errors, status = curl_output(...)), as Sorbet doesn't understand implicit array conversion. As suggested by Markus, I've switched these areas to use #stdout, #stderr, and #status. This requires the use of an intermediate variable (result) in some cases but this was a fairly straightforward substitution.

I also had to refactor how Cask::URL::BlockDSL::PageWithURL works. Currently it uses page.extend PageWithURL to add a url attribute but this reworks it to subclass String SimpleDelegator and use an initialize method instead. This achieves the same goal but in a way that Sorbet can understand.


Besides that, this adds more tests to curl_spec.rb to increase test coverage. This brings almost all of the methods that don't make network requests up to 100% line and branch coverage (the exception being some guards in parse_curl_output that shouldn't happen under normal circumstances). Overall coverage changes are as follows:

brew tests Command Line % Branch %
--only utils/curl before 47.55 32.20
--only utils/curl after 55.24 40.23
--online before 64.91 47.46
--online after 67.83 53.45

In the process of writing more tests for parse_curl_response, I made some tweaks to remove checks for conditions that shouldn't ever be true (e.g., match["code"] isn't optional, so it will be present if HTTP_STATUS_LINE_REGEX matches) and to refactor some others. I contributed this method a while back (9171eb2), so this is me coming back to clarify some behavior.


Though this is worthwhile in its own right (#17297), this is a prelude to some potential curl_args changes to unblock a livecheck feature that I've been working on for some time.

@samford samford changed the title Curl: use typed: strict Curl: Use typed: strict Jan 11, 2025
@issyl0
Copy link
Member

issyl0 commented Jan 12, 2025

Thanks for this! I have a WIP branch for making all of utils strictly typed, but I’d not made it to this one yet!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks @samford! Looking good so far.

Library/Homebrew/utils/curl.rb Show resolved Hide resolved
@samford samford force-pushed the curl-typed-strict branch 2 times, most recently from 77235c2 to 555981c Compare January 13, 2025 19:45
@samford
Copy link
Member Author

samford commented Jan 13, 2025

Changes in the latest push:

  • I switched the args/extra_args type from T.untyped to String, per @dduugg's comment. We could use T.any(Pathname, String) but there was only one pathname that I've seen so far, so I added a #to_s call instead. Similarly, there were a few areas where a URI object is used, so I added #to_s calls to those as well (to bring them in line with existing call sites where we're doing the same for other URIs). These are curl CLI arguments, so there's maybe something to be said for simply ensuring that these values are strings instead of permitting Pathname or URI::Generic.
  • I refactored areas where we call a Utils::Curl method and use array destructuring on a SystemCommand::Result return value (e.g., output, errors, status = curl_output(...)) to use #stdout, #stderr, and #status instead of my previous #to_a approach, per @reitermarkus's comments. I was curious if this would lead to increased verbosity but it ended up being a fairly straightforward substitution (i.e., pretty much the same number of lines).

This passes brew typecheck and brew tests --online but there may be issues that are only surfaced at runtime, so it would probably be a good idea for folks to do some manual testing with commands that interact with curl before merging.


Thanks for this! I have a WIP branch for making all of utils strictly typed, but I’d not made it to this one yet!

I looked back at some old Git stashes and apparently I've been intermittently working on this since at least 2022 (there were some challenges along the way, to say the least 😆). I finally finished it late last year and I had been meaning to create a PR but hadn't gotten around to it yet. I saw one of your Sorbet PRs the other day and thought, "Oh, I bet Issy's getting close to curl; I better get this out the door." Anyway, I'm glad I managed to save you the trouble!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Should be good once @dduugg's comment addressed!

@samford samford force-pushed the curl-typed-strict branch 2 times, most recently from 44d25b6 to a7bc571 Compare January 14, 2025 13:12
This upgrades `utils/curl.rb` to `typed: strict`, which requires
a number of changes to pass `brew typecheck`. The most
straightforward are adding type signatures to methods, adding type
annotations (e.g., `T.let`) to variables that need them, and ensuring
that methods always use the expected return type.

I had to refactor areas where we call a `Utils::Curl` method and use
array destructuring on a `SystemCommand::Result` return value
(e.g., `output, errors, status = curl_output(...)`), as Sorbet
doesn't understand implicit array conversion. As suggested by Markus,
I've switched these areas to use `#stdout`, `#stderr`, and `#status`.
This requires the use of an intermediate variable (`result`) in some
cases but this was a fairly straightforward substitution.

I also had to refactor how `Cask::URL::BlockDSL::PageWithURL` works.
It currently uses `page.extend PageWithURL` to add a `url` attribute
but this reworks it to subclass `SimpleDelegator` and use an
`initialize` method instead. This achieves the same goal but in a way
that Sorbet can understand.
This adds more tests to `curl_spec.rb` to increase test coverage.
This brings almost all of the methods that don't make network
requests up to 100% line and branch coverage (the exception being
some guards in `parse_curl_output` that shouldn't happen under
normal circumstances).

In the process of writing more tests for `parse_curl_response`, I
made some tweaks to remove checks for conditions that shouldn't ever
be true (e.g., `match["code"]` isn't optional, so it will be present
if `HTTP_STATUS_LINE_REGEX` matches) and to refactor some others. I
contributed this method a while back (9171eb2), so this is me coming
back to clarify some behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants