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

cask/utils: Make more noise when encountering undefined methods #15149

Merged
merged 1 commit into from Apr 4, 2023

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Apr 4, 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?

- https://github.com/Homebrew/homebrew-cask/actions/runs/4608585102/jobs/8144571098
  introduced syntax errors for the `mattermost` cask (`autoupdates`
  instead of `auto_updates`), but CI didn't fail so we didn't notice
  until it shipped to users and broke `brew update`.
@issyl0 issyl0 force-pushed the more-noisy-on-unknown-cask-methods branch from 0727175 to 7cfa544 Compare April 4, 2023 16:09
@@ -104,7 +104,7 @@ def self.method_missing_message(method, token, section = nil)
message << "during #{section} " if section
message << "on Cask #{token}."

opoo "#{message}\n#{error_message_with_suggestions}"
ofail "#{message}\n#{error_message_with_suggestions}"
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me but @Homebrew/cask folks may have an opinion about whether we want to keep opoo/a warning for users rather than maintainers here?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer we throw an error for everyone. No busted syntax allowed ❌

@issyl0 issyl0 merged commit 8adaada into Homebrew:master Apr 4, 2023
22 checks passed
@issyl0 issyl0 deleted the more-noisy-on-unknown-cask-methods branch April 4, 2023 16:54
@reitermarkus
Copy link
Member

Does this now fail for casks that are already installed? We don't want to fail reading installed casks.

For example, if we remove appcast eventually, installed casks should not fail with a unknown method error when we upgrade them.

@MikeMcQuaid
Copy link
Member

For example, if we remove appcast eventually, installed casks should not fail with a unknown method error when we upgrade them.

Agreed but: feels like they shouldn't warn in this situation, either?

@reitermarkus
Copy link
Member

No, it should work the same as odisabled which is also a no-op for installed formulae and casks but fails otherwise.

@MikeMcQuaid
Copy link
Member

@reitermarkus Agreed!
@issyl0 @p-linnane thoughts?

@p-linnane
Copy link
Member

That's fine with me.

@github-actions github-actions bot added the outdated PR was locked due to age label May 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2023
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.

None yet

4 participants