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

Add #except under Ruby 3 #545

Merged
merged 2 commits into from Nov 8, 2021
Merged

Add #except under Ruby 3 #545

merged 2 commits into from Nov 8, 2021

Conversation

jackjennings
Copy link
Contributor

@jackjennings jackjennings commented Apr 7, 2021

Ruby 3 adds the Hash#except method, which should return the correct Hashie object instance when called on a Mash or object using the Hashie::Extensions::IndifferentAccess mixin.

This change implements #except in both of those locations and adds the corresponding specs.

@jackjennings jackjennings marked this pull request as ready for review April 7, 2021 00:56
@michaelherold
Copy link
Member

Hi @jackjennings, thanks for the submission! We're in a bit of a pickle right now because we haven't merged #543 yet to move over to Actions from Travis. When one of us has the chance we'll get that merged so we can rebase this and double-check that everything is green.

I don't see anything concerning and agree this should be in the gem. There's a little bit of ambiguity in the way you're calling #slice that I just want to make sure about.

Also, we haven't been testing against 3.0 so I don't know if there anything that we need to clear up for the keyword argument changes.

Anyway, I wanted to get you a response since I was looking at my email when you submitted the PR. Please bear with us!

@jackjennings
Copy link
Contributor Author

jackjennings commented Apr 7, 2021

@michaelherold no rush on my end. This was an issue that arose in an app I'm working on that uses Searchkick, which extends hashie's mash, and calling except starts returning a hash in Rails 6.1 under Ruby 3. I've worked around it (for anyone that lands here before merging) by just wrapping the return value in a new Mash.

The except method definition is actually lifted from Rails' core_ext, but I agree with you that this isn't super clear. A potential alternative would be to not use the slice implementation, but drop down to the native implementation and then re-wrap in a new instance (the same implementation as slice, actually):

def except(*keys)
  string_keys = keys.map { |key| convert_key(key) }
  self.class.new(super(*string_keys))
end

There is some elegance in defining except as a kind of inverse of slice, but let me know what your preference is and I'm happy to update this PR.

@jackjennings
Copy link
Contributor Author

@michaelherold which implementation do you prefer? I’ll update this PR to be ready to merge now that the CI work is done

@michaelherold
Copy link
Member

I'm good with the current implementation that you have. I'm not sure what I was thinking back then but I don't see anything ambiguous now. 😅

I think we just need a rebase!

@jackjennings
Copy link
Contributor Author

I ended up swapping it out with the super-based implementation to avoid some additional method calls with re-converting keys into strings. Rebased with latest changes, but looks like a maintainer needs to approve running workflows.

@jackjennings
Copy link
Contributor Author

@michaelherold looks like danger failed on my force-pushed change — not really sure what to do…?

CHANGELOG.md Outdated Show resolved Hide resolved
@jackjennings jackjennings force-pushed the master branch 2 times, most recently from 4865e42 to 04e8a19 Compare July 13, 2021 18:26
@dblock
Copy link
Member

dblock commented Jul 13, 2021

Noticed some extra quote in https://github.com/hashie/hashie/pull/545/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edL43, maybe you can fix that one too? :) Obviously won't hold this PR for that. I kicked off CI.

@jackjennings
Copy link
Contributor Author

@dblock — not sure what you mean by an extra quote; the link that you posted doesn't point to a specific line.

@dblock
Copy link
Member

dblock commented Jul 28, 2021

@dblock — not sure what you mean by an extra quote; the link that you posted doesn't point to a specific line.

Yeah sorry not sure what happened here, search for [#533](https://github.com/hashie/hashie/pull/533) in CHANGELOG, it's got a quote in to_json that messes up formatting.

@jackjennings
Copy link
Contributor Author

@dblock fixed it.

@dblock
Copy link
Member

dblock commented Jul 30, 2021

Dunno what's up with Danger. @michaelherold cool to merge with me.

@jackjennings
Copy link
Contributor Author

@michaelherold good to get this in?

@jackjennings
Copy link
Contributor Author

jackjennings commented Oct 20, 2021

@michaelherold Can this be merged in, or should I close it? I know this is probably not particularly pressing.

@dblock
Copy link
Member

dblock commented Oct 21, 2021

@jackjennings I am a little worried about the failing integration test, can you please take a look? Force push let's get CI passing?

Ruby 3 adds the Hash#except method, which should return the correct
Hashie object instance when called on a Mash or object using the
Hashie::Extensions::IndifferentAccess mixin.
@dblock
Copy link
Member

dblock commented Nov 8, 2021

Verified that this will be 🍏 in #552.

@dblock dblock merged commit 796f944 into hashie:master Nov 8, 2021
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.

None yet

3 participants