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.

Sign up for GitHub

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

Jump to bottom

Extend Rake with a more elegant and reliable way #1517

Merged
merged 2 commits into from
Jul 29, 2021
Merged

Extend Rake with a more elegant and reliable way #1517

merged 2 commits into from
Jul 29, 2021

Conversation

knapo
Copy link
Contributor

@knapo knapo commented Jul 28, 2021 •
edited

Extend Rake with a more elegant and reliable way

Closes #1520

Description

Per 4dc603b#r54079829

Overwriting methods with aliasing is is rather a bad practice and, since it may cause conflicts, circular dependencies and infinite loops. Module#prepend is more elegant and reliable.

@st0012
Copy link
Collaborator

st0012 commented Jul 28, 2021

As I mentioned in 4dc603b#r54080150, using prepend doesn't guarantee conflict-free.

in fact, I used prepend over alias when patching Net::HTTP. but the conflicts aren't that few, you can see #1427 for more detail.

I choose alias for the rake integration because display_error_message was patched this way since the sentry-raven era and it seemed to work fine. so I didn't want to change it and potentially introduce another set of conflicts.

Can you tell me what caused the conflicts in your application?

@knapo
Copy link
Contributor Author

knapo commented Jul 29, 2021 •
edited

@st0012

Can you tell me what caused the conflicts in your application?

My other gem extends Rake::Task with prepend, what doesn't cooperates with aliasing and getting a stack level too deep (SystemStackError). It's basically same case as in the example I shared .

@st0012 st0012 self-assigned this Jul 29, 2021
@st0012 st0012 added this to In progress in 4.x via automation Jul 29, 2021
@st0012 st0012 added this to the 4.7.0 milestone Jul 29, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2021 •
edited

Codecov Report

Merging #1517 (65b280f) into master (8387282) will increase coverage by 0.55%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1517      +/-   ##
==========================================
+ Coverage   98.20%   98.75%   +0.55%     
==========================================
  Files         218      123      -95     
  Lines       10607     6771    -3836     
==========================================
- Hits        10417     6687    -3730     
+ Misses        190       84     -106
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/rake.rb 55.55% <50.00%> (+2.61%) ⬆️
sentry-raven/spec/support/test_rails_app/app.rb
...ntry-raven/lib/raven/helpers/deprecation_helper.rb
...raven/integrations/rails/controller_transaction.rb
...c/raven/processors/removecirculareferences_spec.rb
sentry-raven/lib/raven/client.rb
sentry-raven/spec/raven/event_spec.rb
sentry-raven/lib/raven/utils/deep_merge.rb
...try-raven/lib/raven/interfaces/single_exception.rb
...ry-raven/spec/raven/interfaces/stack_trace_spec.rb
... and 86 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 8387282...65b280f. Read the comment docs.

st0012
st0012 approved these changes Jul 29, 2021
View reviewed changes
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 fix 👍

4.x automation moved this from In progress to Reviewer approved Jul 29, 2021
@st0012 st0012 merged commit 9901068 into getsentry:master Jul 29, 2021
4.x automation moved this from Reviewer approved to Done Jul 29, 2021
@knapo knapo deleted the extend-rake-in-a-more-elegant-way branch July 29, 2021 12:48
@st0012 st0012 modified the milestones: 4.7.0, 4.6.4 Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@st0012 st0012 st0012 approved these changes

Assignees

@st0012 st0012

Labels
sentry-ruby
Projects
4.x
  
Done
Milestone
4.6.4
Development

Successfully merging this pull request may close these issues.

stack level too deep (SystemStackError) after upgrading from 4.6.1 to 4.6.2
3 participants
@knapo @st0012 @codecov-commenter