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.

Sign up for GitHub

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

Jump to bottom

sorbet: Make more files (that have public APIs defined in them) typed: strict #17636

Merged
merged 10 commits into from
Jul 12, 2024
Merged

sorbet: Make more files (that have public APIs defined in them) typed: strict #17636

merged 10 commits into from
Jul 12, 2024

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Jul 5, 2024

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

@issyl0 issyl0 force-pushed the sorbet-strict-public-apis branch from 8dfd2b4 to 644ff02 Compare July 5, 2024 17:16
@issyl0 issyl0 force-pushed the sorbet-strict-public-apis branch from 644ff02 to 5abd08b Compare July 5, 2024 17:17
@issyl0 issyl0 force-pushed the sorbet-strict-public-apis branch from 5abd08b to cd18694 Compare July 5, 2024 17:48
@issyl0 issyl0 marked this pull request as draft July 5, 2024 17:59
MikeMcQuaid
MikeMcQuaid reviewed Jul 5, 2024
View reviewed changes
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.

Looking great so far!

Library/Homebrew/development_tools.rb Outdated Show resolved Hide resolved
Library/Homebrew/development_tools.rb Outdated Show resolved Hide resolved
Library/Homebrew/unpack_strategy.rb Outdated Show resolved Hide resolved
Library/Homebrew/unpack_strategy.rb Show resolved Hide resolved
Library/Homebrew/unpack_strategy.rb Outdated Show resolved Hide resolved
Library/Homebrew/unpack_strategy.rb Outdated Show resolved Hide resolved
@issyl0 issyl0 force-pushed the sorbet-strict-public-apis branch from bab0dda to 0a18f77 Compare July 6, 2024 14:22
@issyl0 issyl0 mentioned this pull request Jul 8, 2024
Merged
7 tasks
MikeMcQuaid
MikeMcQuaid approved these changes Jul 8, 2024
View reviewed changes
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.

Looks great, thanks @issyl0!

Library/Homebrew/development_tools.rb Show resolved Hide resolved
@issyl0
Copy link
Member Author

issyl0 commented Jul 8, 2024

Something with the new UnpackStrategy is causing a Cask test to fail that I can't figure out why it would have become more verbose. 🤔

@dduugg
Copy link
Sponsor Member

dduugg commented Jul 8, 2024 •
edited
Loading

Something with the new UnpackStrategy is causing a Cask test to fail that I can't figure out why it would have become more verbose. 🤔

I think the test failure involves the combination of changes to Library/Homebrew/unpack_strategy/zip.rb and Library/Homebrew/unpack_strategy.rb (reverting them both resolved the failure locally for me). I haven't been able to debug further, but thought that might be useful to you.

@issyl0
Copy link
Member Author

issyl0 commented Jul 9, 2024

Yeah I got that far too. 😅

@issyl0
All the extract_to_dirs return void now
9c64309
@issyl0
unpack_strategy: Fix ref_type type (Symbol, not String)
bffb470 
- I put a debugger call in the test that was failing.
- Running the install command at that debug prompt and lo,
  the typing bug was staring me in the face:

```
Error: An exception occurred within a child process:
  TypeError: Parameter 'ref_type': Expected type T.nilable(String), got type Symbol with value :branch
Caller: /opt/homebrew/Library/Homebrew/vendor/bundle/ruby/3.3.0/gems/sorbet-runtime-0.5.11471/lib/types/private/methods/call_validation.rb:215
Definition: /opt/homebrew/Library/Homebrew/unpack_strategy.rb:102 (UnpackStrategy.detect)
```
@issyl0
unpack_strategy/*: Convert to Sorbet typed: strict
edb8055
@issyl0 issyl0 force-pushed the sorbet-strict-public-apis branch from e09145e to edb8055 Compare July 12, 2024 01:18
@issyl0 issyl0 marked this pull request as ready for review July 12, 2024 02:02
@issyl0
Copy link
Member Author

issyl0 commented Jul 12, 2024

This is finally green!

@MikeMcQuaid
Copy link
Member

Thanks @issy! Good to self-merge.

1 similar comment
@MikeMcQuaid
Copy link
Member

Thanks @issy! Good to self-merge.

@issyl0 issyl0 merged commit 1479013 into master Jul 12, 2024
25 checks passed
@issyl0 issyl0 deleted the sorbet-strict-public-apis branch July 12, 2024 16:25
@issyl0 issyl0 mentioned this pull request Jul 12, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dduugg dduugg dduugg left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@issyl0 @dduugg @MikeMcQuaid