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

Formula, BuildError: Update type signatures #16002

Merged
merged 1 commit into from Sep 14, 2023

Conversation

samford
Copy link
Member

@samford samford commented Sep 13, 2023

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

We're seeing type errors when building formulae that use something like xcodebuild ..., "-arch", Hardware::CPU.arch, since CPU.arch is a symbol. We've been addressing these issues by calling #to_s on the value but there was some talk about simply expanding the type signatures to accommodate anything that will be cast to a String.

There's maybe still an argument to be made for doing string conversion in formulae but expanding the type signatures will resolve a number of existing type errors if we simply want to rely on implicit type casting.

Past that, this also updates the type signature for BuildError to align with the #system signature changes, as we receive a new type error otherwise.

@samford samford added the critical Critical change which should be shipped as soon as possible. label Sep 13, 2023
@samford
Copy link
Member Author

samford commented Sep 13, 2023

I'm looking into the new brew typecheck failures now that I have brew typecheck working on Sonoma (we currently have to set HOMEBREW_FORCE_VENDOR_RUBY=1).

Edit: I updated the type signature for BuildError to align with the change to #system and now brew typecheck passes.

We're seeing type errors when building formulae that use something
like `xcodebuild ..., "-arch", Hardware::CPU.arch`, since `CPU.arch`
is a symbol. We've been addressing these issues by calling `#to_s` on
the value but there was some talk about simply expanding the type
signatures to accommodate anything that will be cast to a `String`.

There's maybe still an argument to be made for doing string conversion
in formulae but expanding the type signatures will resolve a number of
existing type errors if we simply want to rely on implicit type
casting.

Past that, this also updates the type signature for `BuildError` to
align with the `#system` signature changes, as we receive a type error
otherwise.
@samford samford force-pushed the update-xcodebuild-type-signature branch from e34a053 to 578c935 Compare September 13, 2023 23:16
@samford samford changed the title Formula: Update type signatures Formula, BuildError: Update type signatures Sep 13, 2023
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Symbol looks like a good option

Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Yeah to_str kinda sucks when you deal with explicit typing.

I'm guessing Version might appear in a few places too.

@Bo98 Bo98 merged commit fe07ced into Homebrew:master Sep 14, 2023
24 checks passed
@MikeMcQuaid
Copy link
Member

Thanks @samford, looks good!

@samford samford deleted the update-xcodebuild-type-signature branch September 14, 2023 15:07
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants