-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add ErrorReporter unexpected method #264
Conversation
It's kind of funny this fails the runtime checks even with the version constraint added 🙈
|
@@ -472,4 +472,15 @@ class ActiveSupport::ErrorReporter | |||
).void | |||
end | |||
def report(error, handled: true, severity: T.unsafe(nil), context: T.unsafe(nil), source: T.unsafe(nil)); end | |||
|
|||
# @version >= 7.2.0.beta1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you win the award for the very first version annotation? 🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we want to handle runtime checks? I think CI will use the latest version however beta versions can only be specified in the Gemfile?
There's also the case of annotations with an incompatible version, < 7.2.0beta1
. Should we just use @method_missing
instead of dealing with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to think more about this today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is fine thanks to Shopify/tapioca#1585 but I'll leave it to @egiurleo before merging. We may wanna test versioning with apps that use lower Rails versions as we merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's merge this and I'll add a note to our meeting on Monday to talk about it.
e330d47
to
f1d96c5
Compare
With Rails 7.2 out this now passes CI. |
Type of Change
Changes