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

Avoid causing NoMethodError for Sentry.* calls when the SDK is not inited #1713

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Feb 5, 2022

Closes #1706

@st0012 st0012 added this to the 5.1.0 milestone Feb 5, 2022
@st0012 st0012 self-assigned this Feb 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2022

Codecov Report

Merging #1713 (f4dee80) into master (42afb25) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1713      +/-   ##
==========================================
+ Coverage   98.42%   98.44%   +0.02%     
==========================================
  Files         139      140       +1     
  Lines        7944     7990      +46     
==========================================
+ Hits         7819     7866      +47     
+ Misses        125      124       -1     
Impacted Files Coverage Δ
sentry-rails/spec/support/test_rails_app/app.rb 97.77% <100.00%> (+0.01%) ⬆️
sentry-ruby/lib/sentry-ruby.rb 95.45% <100.00%> (+1.11%) ⬆️
sentry-ruby/spec/initialization_check_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/breadcrumb.rb 100.00% <0.00%> (+3.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42afb25...f4dee80. Read the comment docs.

@st0012 st0012 force-pushed the fix-#1706 branch 2 times, most recently from 5295794 to c8602e6 Compare February 5, 2022 10:41
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

thx :)

@lewispb
Copy link
Contributor

lewispb commented Feb 7, 2022

This PR looks good as-is but I was wondering if it was possible to avoid the explicit return unless initialized? calls everywhere.

One idea I had to do that would be to append something like this to sentry-ruby.rb (not tested):

unless Sentry.initialized?
  klass_methods = Sentry.methods(false)
  Sentry.singleton_class.prepend(Module.new do
    klass_methods.each do |method_name|
      define_method(method_name) do
        nil
      end
    end
  end)
end

That way it'd automatically cover future changes too.

@st0012
Copy link
Collaborator Author

st0012 commented Feb 7, 2022

@lewispb Yeah I've thought about that but I didn't think it's a good idea because:

  1. The interfaces are pretty static. We haven't introduced any new method for months. So the ease of adding new APIs is not the most important thing here.
  2. Whichever meta-programming route we take, it won't be very expressive about its intention.
  3. It's more common that users/maintainers debug or investigate individual API methods than examining sentry-ruby.rb altogether. So I think explicitly stating return unless initialized? in every method makes them more understandable in most cases.

@lewispb
Copy link
Contributor

lewispb commented Feb 7, 2022

@st0012 those are all good points 👍 thanks for explaining your thinking here.

@st0012 st0012 merged commit ee5eadc into master Feb 7, 2022
@st0012 st0012 deleted the fix-#1706 branch February 7, 2022 21:57
@st0012
Copy link
Collaborator Author

st0012 commented Feb 7, 2022

@lewispb np, thanks for the advice too. I sincerely appreciate that you check my code and provide suggestions from time to time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't cause NoMethodError when calling Sentry.* with an uninitialized SDK
4 participants