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

Silence some ruby warnings #1504

Merged
merged 1 commit into from
Jul 24, 2021

Conversation

ojab
Copy link
Contributor

@ojab ojab commented Jul 8, 2021

Description

Describe your changes:

…/sentry-ruby-core-4.6.1/lib/sentry/core_ext/object/duplicable.rb:122: warning: method redefined; discarding old duplicable?
…/activesupport-6.1.4/lib/active_support/core_ext/object/duplicable.rb:36: warning: previous definition of duplicable? was here
…/sentry-ruby-core-4.6.1/lib/sentry/core_ext/object/deep_dup.rb:31: warning: method redefined; discarding old deep_dup
…/activesupport-6.1.4/lib/active_support/core_ext/object/deep_dup.rb:29: warning: previous definition of deep_dup was here
…/sentry-ruby-core-4.6.1/lib/sentry/transport/configuration.rb:14: warning: method redefined; discarding old transport_class=
…/sentry-ruby-core-4.6.1/lib/sentry/event.rb:78: warning: method redefined; discarding old timestamp=
…/sentry-ruby-core-4.6.1/lib/sentry/event.rb:82: warning: method redefined; discarding old level=
…/sentry-ruby-core-4.6.1/lib/sentry/event.rb:102: warning: method redefined; discarding old type
…/sentry-ruby-core-4.6.1/lib/sentry/transaction_event.rb:17: warning: method redefined; discarding old start_timestamp=
…/sentry-ruby-core-4.6.1/lib/sentry/transaction_event.rb:21: warning: method redefined; discarding old type

Not sure if we should undef_method or just not define our variants. As a corporation you probably want undef_method for extra-defined behavior.
Also could add warning gem to devdeps and enforce no-warnings via CI if desired.

@ojab
Copy link
Contributor Author

ojab commented Jul 8, 2021

Sending to review without green CI due to First-time contributors need a maintainer to approve running workflows.. Four specs are failing locally both on master and here.

Also not sure if changelog here is needed and which CHANGELOG.md should it go to.

@ojab ojab marked this pull request as ready for review July 8, 2021 19:52
@ojab ojab force-pushed the silence_redefinition_warnings branch 3 times, most recently from 8386c50 to dd1c32c Compare July 8, 2021 21:12
@ojab ojab changed the title Silence method redifinition warnings Silence method redefinition warnings Jul 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2021

Codecov Report

Merging #1504 (0adc8d0) into master (dbd2cc8) will increase coverage by 0.54%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
+ Coverage   98.19%   98.74%   +0.54%     
==========================================
  Files         218      123      -95     
  Lines       10598     6760    -3838     
==========================================
- Hits        10407     6675    -3732     
+ Misses        191       85     -106     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/configuration.rb 97.96% <100.00%> (ø)
sentry-ruby/lib/sentry/core_ext/object/deep_dup.rb 94.11% <100.00%> (+0.36%) ⬆️
...ntry-ruby/lib/sentry/core_ext/object/duplicable.rb 70.21% <100.00%> (+0.64%) ⬆️
sentry-ruby/lib/sentry/transport/configuration.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/configuration_spec.rb 98.24% <100.00%> (ø)
...y/spec/sentry/interfaces/request_interface_spec.rb 99.14% <100.00%> (ø)
sentry-ruby/spec/sentry/net/http_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/transport_spec.rb 100.00% <100.00%> (ø)
...raven/spec/raven/processors/utf8conversion_spec.rb
sentry-raven/lib/raven/processor/cookies.rb
... and 93 more

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 dbd2cc8...0adc8d0. Read the comment docs.

Copy link
Collaborator

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍
The changes generally look good but I want to cleanup Event and TransactionEvent's attributes myself. Can you exclude them?

And regarding changeslog, can you update the top-level one? thanks.

@st0012 st0012 added this to the 4.7.0 milestone Jul 17, 2021
@ojab ojab force-pushed the silence_redefinition_warnings branch from dd1c32c to 0adc8d0 Compare July 24, 2021 11:21
@ojab
Copy link
Contributor Author

ojab commented Jul 24, 2021

The changes generally look good but I want to cleanup Event and TransactionEvent's attributes myself. Can you exclude them?

Done. Any ETA on this or GH issue that I could follow?

@ojab ojab requested a review from st0012 July 24, 2021 11:22
@ojab ojab changed the title Silence method redefinition warnings Silence some ruby warnings Jul 24, 2021
after(:all) do
Net.send(:const_set, :BufferedIO, original_buffered_io)
end
before { stub_const('Net::BufferedIO', Net::WebMockNetBufferedIO) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@st0012
Copy link
Collaborator

st0012 commented Jul 24, 2021

Done. Any ETA on this or GH issue that I could follow?

I'll add a PR for that in a week and include an changelog entry for this PR as well.

@st0012 st0012 merged commit 02401e9 into getsentry:master Jul 24, 2021
@ojab
Copy link
Contributor Author

ojab commented Jul 24, 2021

Ugh, I have CHANGELOG entry locally, but it's not committed 🤦

@st0012
Copy link
Collaborator

st0012 commented Jul 24, 2021

I've added #1513 as a continuation of the work.

@st0012 st0012 modified the milestones: 4.7.0, 4.6.3 Jul 26, 2021
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.

3 participants