Skip to content

Cask: Initialize yet more instance variables #20100

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

Merged
merged 1 commit into from
Jun 13, 2025

Conversation

samford
Copy link
Member

@samford samford commented Jun 13, 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?

I previously added some instance variables in Cask::DSL to its initialize method (#19806, #19808) but I missed some last time, so we still see warnings like The class Cask::DSL reached 8 shape variations, instance variables accesses will be slower and memory usage increased. It is recommended to define instance variables in a consistent order, for instance by eagerly defining them all in the #initialize method.

This initializes more instance variables in Cask classes to resolve other situations where this warning may occur. I've been testing this for a while and haven't see any warnings with these changes but there's always a chance that there's still more work to be done.

I previously added some instance variables in `Cask::DSL` to its
`initialize` method but I missed some last time, so we still see
warnings like `The class Cask::DSL reached 8 shape variations,
instance variables accesses will be slower and memory usage increased.
It is recommended to define instance variables in a consistent order,
for instance by eagerly defining them all in the #initialize method.`

This initializes more instance variables in `Cask` classes to resolve
other situations where this warning may occur. I've been testing this
for a while and haven't see any warnings with these changes but
there's always a chance that there's still more work to be done.
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! In future: probably worth considering bumping these to typed: strict, too, which will enforce this.

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jun 13, 2025
Merged via the queue into master with commit cba6ecc Jun 13, 2025
33 checks passed
@MikeMcQuaid MikeMcQuaid deleted the cask-initialize-yet-more-instance-variables branch June 13, 2025 13:05
@samford
Copy link
Member Author

samford commented Jun 13, 2025

@MikeMcQuaid I mentioned it internally just now but I have some in-progress work to expand type signatures in Cask::DSL (it's quite a rabbit hole). When I last left it, there were some areas that I couldn't handle without Doug's proposed changes to the Forwardables compiler but there's no PR for that yet. I'll take a look at my local branch again and see if I can create a PR for what I've done so far (it's basically everything else), though we may not be able to bump Cask::DSL to typed: strict until the Forwardables changes are merged.

I made some additional changes to Forwardables in the process (handling additional type signatures), so I'll check with Doug about how we should move that forward when I dig back into this (sometime after I wrap up the UsesOnSystem PR).

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.

2 participants