-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Play nice with sorbet and tapioca #2189
Comments
@Velrok some of our other patches (redis/net-http) also use prepend, how does that work then? |
Hmm not sure. bin/tapioca dsl Which will start the rails app and inspect the classes at runtime. Maybe the other patches are not loaded? Is sorbet compatible something you are interested in supporting? |
@Velrok I haven't really thought about it since you're the first person who asked. |
This is not tested but I found myself in the same situation and thought about adding something like this to sig { params(slug: String, monitor_config: Sentry::Cron::MonitorConfig).void }
def self.sentry_monitor_check_ins(slug:, monitor_config:)
around_perform do |_job, block|
check_in_id = Sentry.capture_check_in(slug, :in_progress, monitor_config:)
start = Sentry.utc_now.to_i
begin
block.call
duration = Sentry.utc_now.to_i - start
Sentry.capture_check_in(
slug,
:ok,
check_in_id:,
duration:,
monitor_config:,
)
rescue StandardError
duration = Sentry.utc_now.to_i - start
Sentry.capture_check_in(
slug,
:error,
check_in_id:,
duration:,
monitor_config:,
)
raise
end
end
end It
By having the same interface you should be able to replace that method with the sentry mixin if/when sentry starts playing nice with Sorbet. In the end I made another solution so I never tested this in production. I did run some tests in development and that seemed to work just fine. |
I want to first clarify that while Sorbet doesn't support Secondly, if something crashes when running And since it's a runtime issue, it's likely caused by things not being loaded properly. The solution could be:
However, without further information we can't properly suggest a better solution here. Can you please add more information, like:
|
@st0012 thanks for the feedback. I do believe the crash Sorbet+Tapioca users experience here has to do with prepend and is not a loading issue. Here's the information you requested: Versions:
Error backtrace:
Tapioca config: gem:
# Add your `gem` command parameters here:
#
exclude:
[]
# - red-arrow
# - red-parquet
# doc: true
# workers: 5
dsl:
# Add your `dsl` command parameters here:
#
# exclude:
# - SomeGeneratorName
# workers: 5 The sorbet-runtime method that raises the error looks like this: # Replaces a method, either by overwriting it (if it is defined directly on `mod`) or by
# overriding it (if it is defined by one of mod's ancestors). If `original_only` is
# false, returns a ReplacedMethod instance on which you can call `bind(...).call(...)`
# to call the original method, or `restore` to restore the original method (by
# overwriting or removing the override).
#
# If `original_only` is true, return the `UnboundMethod` representing the original method.
def self.replace_method(mod, name, original_only=false, &blk)
original_method = mod.instance_method(name)
original_visibility = visibility_method_name(mod, name)
original_owner = original_method.owner
mod.ancestors.each do |ancestor|
break if ancestor == mod
if ancestor == original_owner
# If we get here, that means the method we're trying to replace exists on a *prepended*
# mixin, which means in order to supersede it, we'd need to create a method on a new
# module that we'd prepend before `ancestor`. The problem with that approach is there'd
# be no way to remove that new module after prepending it, so we'd be left with these
# empty anonymous modules in the ancestor chain after calling `restore`.
#
# That's not necessarily a deal breaker, but for now, we're keeping it as unsupported.
raise "You're trying to replace `#{name}` on `#{mod}`, but that method exists in a " \
"prepended module (#{ancestor}), which we don't currently support."
end
end
overwritten = original_owner == mod
T::Configuration.without_ruby_warnings do
T::Private::DeclState.current.without_on_method_added do
def_with_visibility(mod, name, original_visibility, &blk)
end
end
if original_only
original_method
else
new_method = mod.instance_method(name)
ReplacedMethod.new(mod, original_method, new_method, overwritten, original_visibility)
end
end which leads me to think that the error is intentional and not a bug or a loading issue. |
Thank you for the information, I now understand the problem (also described in great detail in this issue). Unfortunately, I don't think not using prepend is possible because the alternative would be something like I think for now the only way to avoid this is to opt-out the patch that triggers the prepend. Given the class's name has Sentry.init { ... }
Sentry.registered_patches.delete(:sidekiq_cron) |
A related/separate disadvantage of For example: class BaseScheduledJob
sentry_monitor_check_ins
def perform
# this calls Sentry::Cron::MonitorCheckIns::Patch#perform
end
end
class SomeSpecificScheduledJob < BaseScheduledJob
def perform
# this doesn't call Sentry::Cron::MonitorCheckIns::Patch#perform
end
end This is subtle and took a while for me to figure out (I learned some new things!). It makes me feel like |
Describe the idea
It would be nice if the monitor_check_ins.rb wouldn't use
preprend
, because that means we can't adopt it alongside sorbet atm.Why do you think it's beneficial to most of the users
Sorbet is a mature type checker for ruby, which is great to have in large code bases. They decided not to support prepend, which seams reasonable to me.
Possible implementation
Sorry I don't understand prepend well enough to really comment. Couldn't it be wrapped in some sort of block / yield alternative?
The text was updated successfully, but these errors were encountered: