Implement non-destructive standard Hash methods #481
Conversation
BobbyMcWho
force-pushed
the
480-implement-non-destructive-methods
branch
3 times, most recently
from
4eaa260 to
9d77668
Compare
|
I merged #482, rebase and stuff. Will CR, thanks! |
When calling the following non-destructive hash methods: :compact :invert :reject :select :slice :transform_keys :transform_values we would be returned an instance of a standard Hash rather than a Mash (or subclass). This changes that behavior to instead return an instance of the class the method was called on.
As of ruby 2.6, Hash#merge and Hash#merge! allow for multiple hashes to be passed to the method, and will merge each one in the order that they were passed.
BobbyMcWho
force-pushed
the
480-implement-non-destructive-methods
branch
from
9d77668 to
157c3ec
Compare
|
@dblock This has been rebased, and is ready for CR 😸 |
|
I like this a lot. You should update README, CHANGELOG and UPGRADING. I'll be ok with merging this if @michaelherold is ok. |
|
This looks good to me! Thanks for catching these issues. |
|
Can you give me some guidance on the most appropriate place in the changelog for this? Edit: I'm leaning |
|
Yeah, it would really be "changed" since these methods were previously returning a different data type. However, given that that feels like a bug, it could also be considered for "fixed". |
|
I would argue for Fixed if it was just the Ruby version specific methods that needed fixed and we were quick about fixing them post-release, but given how long the current behavior has acted this way, I'd argue that at this point it's safest for everyone to just go with a Major version bump |
|
I am +1 on major version bump. it's the safest. |
|
I merged this. |
Resolves #480
When calling the following non-destructive hash methods:
we would be returned an instance of a standard Hash rather
than a Mash (or subclass). This changes that behavior to
instead return an instance of the class the method was
called on.
Also, as of ruby 2.6, Hash#merge and Hash#merge! allow for multiple hashes
to be passed to the method, and will merge each one in the order that
they were passed. I implemented that in this PR as well, but I could always pull it out into a separate PR if @dblock or @michaelherold think it's necessary to do so.
Ideally this is merged before #479, and I can remove a little scope from that one to specifically target the Rails method it was intended to fix.
Do these changes warrant a major bump since they change the returned type for those methods? I'm not sure returning a Mash instead of a standard Hash would be considered breaking in most cases, but I'm open to thoughts.