Skip to content

Finalize methods that do not support overrides#16640

Merged
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
dduugg:finalize-sigs
Feb 20, 2024
Merged

Finalize methods that do not support overrides#16640
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
dduugg:finalize-sigs

Conversation

@dduugg
Copy link
Copy Markdown
Member

@dduugg dduugg commented Feb 11, 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?

I noticed some methods with comments discouraging overrides. Let's enforce that in code instead.

See: https://sorbet.org/docs/final

Comment thread Library/Homebrew/requirement.rb Outdated
cc: T.nilable(String),
build_bottle: T::Boolean,
bottle_arch: T.nilable(String),
).returns(T.untyped)
Copy link
Copy Markdown
Member Author

@dduugg dduugg Feb 11, 2024

Choose a reason for hiding this comment

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

Note that this method currently returns nil or T::Private::Types::Void::VOID, which is clumsy/incorrect to encode in a type signature. However, we currently have a test that relies on this being nil/non-nil depending on circumstance, so I didn't undertake a refactor. Feel free to weigh in if you want this to be resolved by other means though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is kinda weird, since ENV#prepend_path can technically return nil masked as T::Private::Types::Void::VOID, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd say this should always return nil, or we just remove the test and make it .void.

Probably the latter since we don't have any test/usage that uses the return value anyways.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed that we should probably update the test here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I made the return type consistently void, and also updated the test to assert no error occurs (very roughly equivalent to checking for nil return, in this case).

env: T.nilable(String),
cc: T.nilable(String),
build_bottle: T::Boolean,
bottle_arch: T.nilable(String),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

params are based on https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/ENV.rb#L36-L39 (though I noticed that build_bottle is T.nilable(T::Boolean) in some places)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think T::Boolean is preferable in most cases. We should adjust the call sites to avoid T.nilable(T::Boolean) being passed where this is still the case.

@MikeMcQuaid
Copy link
Copy Markdown
Member

@dduugg what's the latest here?

@dduugg
Copy link
Copy Markdown
Member Author

dduugg commented Feb 19, 2024

@dduugg what's the latest here?

Sorry for the latency here, PTAL. I've updated the signature and test per comments.

Copy link
Copy Markdown
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 good, thanks for the quick update @dduugg!

@MikeMcQuaid MikeMcQuaid merged commit aa58637 into Homebrew:master Feb 20, 2024
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants