Make Hashie play nice with Rails 6 Hash#except method #479
Conversation
There was a problem hiding this comment.
I like this implementation. I'm happy to see that it's not a Rails-specific issue, but that Mash doesn't implement #slice. Since that is a standard library method, we should implement it on Rubies that have it, like you're doing here.
I have a couple of changes I'd like to make to the test suite. If those pass once you merge them, we're good-to-go!
|
@michaelherold e1700b2 adds a railtie for except, I've never made a railtie before, so feel free to let me know if I broke any conventions, the symbol keys test now passes. |
|
As I mentioned in the issue thread, I would rather not ship Rails-specific behavior in Hashie proper if we can avoid it ... but I guess we already have the Railtie for logging. I'm going to think about this for a bit. |
|
@michaelherold more than happy to move that specific piece to a gem if you'd like |
|
Since we already have the Railtie, I think I'm alright with this as-is. For Hashie 4, we'll consider moving all Rails-specific behavior into a companion gem. |
|
Thinking about this a little more, I think we might want to gate redefinition of What do you think, @BobbyMcWho? |
|
@michaelherold I could see it going either way, since I switched it to use super, it will be backwards compatible with the old implementation, but will call an unnecessary |
|
Let's rebase this and deal with it? @BobbyMcWho |
Rails 6 switched to using slice for its except implementation. We'll translate the hash returned back to the class it was called on.
|
Rails 6 seems to have broken the rails integration test. If I find some time I'll try and figure out what exactly is wrong now. |
Using the view_paths was causing issues in rails 6, rendering inline erb prevents those issues.
|
@dblock I've fixed the failing CI |
|
LGTM, @michaelherold ? |
|
|
||
| def index; end | ||
| def index | ||
| render inline: PAGE |
There was a problem hiding this comment.
🤔 Hmm. I think I'm good with this. I don't believe we'll ever do anything where we monkey with view paths so the simplification is good.
There was a problem hiding this comment.
something changed with the rails 6 release that broke this test and I couldn't pinpoint what it was, this was the only semi reasonable way I could get it passing again since I believe that view paths method was deprecated and removed
|
This LGTM, @michaelherold shall we squash & merge? |
|
So can we fix the faraday error in danger here? Hopefully without an |
|
@michaelherold and I both had faraday changes merged and released over the weekend, so hopefully it's finally fixed. I don't know why CI failed to kick off a build just now... |
It's because Octokit hasn't merged and released octokit/octokit.rb#1154 yet, so Danger is pulling a broken combination of Faraday and Octokit. The easiest way to work around this is to lock faraday to 0.15.4 for now for the purposes of Danger. |
|
Hmm, I thought |
|
Ahh @michaelherold it didn't run because I simply dropped the last commit and it already had a run on that commit I'm guessing. Looks like build is passing in my CI if you wanna manually trigger a build here. |
There was a problem hiding this comment.
Gotcha! That makes a lot more sense. Your find with the subclassing constant was a really good one, by the way. 😄
I don't see a reason why we should sit on this any longer. The NoMethodError -> NameError change is still a little concerning to me, but I don't really remember the specifics of what I was thinking with the index_document test so I'm going to let it be. We're testing that an error happens and that seems good enough!
Ha, I saw the error message in this CI run and was like aw man. My solution for 0.16.2 was kinda cobbled together, and I think lostisland/faraday#1043 is a better solution than what I originally got merged 😆 |
|
I merged this. Thanks for hanging in here with us @BobbyMcWho! |
|
Awesome, are we still planning a major bump release of this all since that 1 PR that changed some non-destructive method return types? Will it be a rc1? |
Resolves #476
Because Rails 6 switched their implementation of
#exceptto use slice and slice returns a new ruby hash, we need to translate that back to the class that slice was called on.Let me know if you see any concerns that can be improved upon.