Skip to content

Silence some ruby warnings#1504

Merged
st0012 merged 1 commit intogetsentry:masterfrom
ojab:silence_redefinition_warnings
Jul 24, 2021
Merged

Silence some ruby warnings#1504
st0012 merged 1 commit intogetsentry:masterfrom
ojab:silence_redefinition_warnings

Conversation

@ojab
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

ojab commented Jul 8, 2021 •
edited
Loading

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
Copy Markdown

codecov-commenter commented Jul 9, 2021 •
edited by codecov bot
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.74%. Comparing base (dbd2cc8) to head (0adc8d0).
Report is 760 commits behind head on master.

Additional details and impacted files 
@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

st0012
st0012 reviewed Jul 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
st0012
st0012 approved these changes Jul 24, 2021
View reviewed changes
sentry-ruby/spec/sentry/net/http_spec.rb
after(:all) do
Net.send(:const_set, :BufferedIO, original_buffered_io)
end
before { stub_const('Net::BufferedIO', Net::WebMockNetBufferedIO) }
Copy link
Copy Markdown
Contributor

@st0012 st0012 Jul 24, 2021

Choose a reason for hiding this comment

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

👍

@st0012
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

ojab commented Jul 24, 2021

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

@st0012 st0012 mentioned this pull request Jul 24, 2021
Merged
@st0012
Copy link
Copy Markdown
Contributor

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

Reviewers

1 more reviewer

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring sentry-ruby

Projects

None yet

Milestone

4.6.3

Development

Successfully merging this pull request may close these issues.

3 participants

@ojab @codecov-commenter @st0012