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

Migrate from to_hash to to_h #2351

Merged
merged 2 commits into from
Jul 29, 2024
Merged

Migrate from to_hash to to_h #2351

merged 2 commits into from
Jul 29, 2024

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Jul 23, 2024

As @solnic pointed out in #2350 (comment) to_hash has special meaning in Ruby and could be called implicitly in contexts like double splatting argument. So we should switch to to_h to avoid potential issues.

Additionally, Matz also explained that in the explicit conversion usages (which is the case for the SDK), to_h should be used.

Compatibility Concern

Since this PR changes the serialization interface of almost all SDK's internal components, if users/libs relied on patching to_hash to extend/modify the data, this could become a breaking change for them. And because to_hash has been that interface since the old sentry-raven SDK (for at least 12 years), I think we can assume at least some legacy apps had built customization on top of it.

For this reason, maybe this should be shipped in 6.0?

(In cases where Configuration#async is set, we use Event#to_json_compatible instead, so they won't be affected)

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.63%. Comparing base (7db04c9) to head (0b74d98).

Additional details and impacted files
@@           Coverage Diff            @@
##           6.0-dev    #2351   +/-   ##
========================================
  Coverage    98.63%   98.63%           
========================================
  Files          204      204           
  Lines        13286    13286           
========================================
+ Hits         13104    13105    +1     
+ Misses         182      181    -1     
Components Coverage Δ
sentry-ruby 99.01% <100.00%> (+0.01%) ⬆️
sentry-rails 97.30% <100.00%> (ø)
sentry-sidekiq 97.01% <100.00%> (ø)
sentry-resque 96.79% <100.00%> (ø)
sentry-delayed_job 98.92% <100.00%> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-delayed_job/spec/sentry/delayed_job_spec.rb 100.00% <100.00%> (ø)
sentry-rails/spec/sentry/rails/activejob_spec.rb 99.42% <100.00%> (ø)
...ry/rails/breadcrumbs/active_support_logger_spec.rb 100.00% <100.00%> (ø)
...readcrumbs/monotonic_active_support_logger_spec.rb 91.54% <100.00%> (ø)
...rails/spec/sentry/rails/controller_methods_spec.rb 100.00% <100.00%> (ø)
sentry-rails/spec/sentry/rails/event_spec.rb 100.00% <100.00%> (ø)
...rails/tracing/action_controller_subscriber_spec.rb 100.00% <100.00%> (ø)
...entry/rails/tracing/action_view_subscriber_spec.rb 100.00% <100.00%> (ø)
...try/rails/tracing/active_record_subscriber_spec.rb 100.00% <100.00%> (ø)
...ry/rails/tracing/active_storage_subscriber_spec.rb 93.02% <100.00%> (ø)
... and 42 more

... and 1 file with indirect coverage changes

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Jul 23, 2024

@st0012 thx! agree that this should be shipped in the next major.

I created a 6.0-dev branch
#2352

can you base this rebase this branch on that branch and change the base there?

@solnic
Copy link
Collaborator

solnic commented Jul 24, 2024

Thanks for tackling this! I did not expect you'd address it so quickly ❤️ This is definitely a potentially breaking change, but luckily it's easily fixable in people's codebases.

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.
@st0012 st0012 force-pushed the replace-to-hash branch from 0f2b2e9 to a559f0d Compare July 28, 2024 21:26
@st0012 st0012 changed the base branch from master to 6.0-dev July 28, 2024 21:26
@st0012 st0012 marked this pull request as ready for review July 28, 2024 21:27
@st0012
Copy link
Collaborator Author

st0012 commented Jul 28, 2024

@sl0thentr0py rebased 👍

@sl0thentr0py sl0thentr0py merged commit 329cccb into 6.0-dev Jul 29, 2024
124 checks passed
@sl0thentr0py sl0thentr0py deleted the replace-to-hash branch July 29, 2024 12:33
sl0thentr0py pushed a commit that referenced this pull request Oct 31, 2024
* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.
@sl0thentr0py sl0thentr0py mentioned this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants