Skip to content

Conversation

@seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Feb 13, 2023

Motivation / Background

Add test coverage for existing Object#with_options support for Hash-like objects.

Detail

The current implementation expects "Hash-like" to mean that the argument implements both Hash#deep_merge and #to_hash (to be called explicitly with #to_hash and implicitly with **).

This coverage is an explicit commitment to (what has so far been implicit) support for objects that do not descend from Hash, but implement Hash methods. This includes classes like ActionController::Parameters (which inherits from Object) and others like it that return instances of a class other than Hash when calling deep_merge.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@seanpdoyle seanpdoyle force-pushed the with-options-hash-like branch from b41a17e to 8ed2abd Compare February 13, 2023 20:15
Add test coverage for existing `Object#with_options` support for
`Hash`-like objects.

The current implementation expects "Hash-like" to mean that the argument
implements both `Hash#deep_merge` and `#to_hash` (to be called
explicitly with `#to_hash` and implicitly with `**`).
@seanpdoyle seanpdoyle force-pushed the with-options-hash-like branch from 8ed2abd to 286fa40 Compare February 14, 2023 03:08
@guilleiguaran guilleiguaran merged commit ad71489 into rails:main Feb 15, 2023
@seanpdoyle seanpdoyle deleted the with-options-hash-like branch February 15, 2023 21:09
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