Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants