Skip to content

Remove hashie dependency #1448

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove hashie dependency #1448

wants to merge 1 commit into from

Conversation

etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Jul 20, 2016 •
edited
Loading

Because less dependencies is much better than more.
This is breaking change and I'm open for discussion.

Additional point for doing this is that in our app we are still locked to hashie ~> 2.1.1 (we aren't using it in the project) just because later versions require https://github.com/Maxim-Filimonov/hashie-forbidden_attributes gem. This is kinda weird from my point of view to require "workaround" gem for internal grape dependency.

P.S. Spec suite is green, but I'm not sure that there is no unexpected behavior changes.

@etehtsea
Copy link
Contributor Author

etehtsea commented Jul 20, 2016 •
edited
Loading

Didn't see that it was already made in #1279 and #737
Leaving it open because this PR is based on HashWithIndifferentAccess.

@etehtsea
Remove hashie dependency
dd74cb4 
Because less dependencies is much better than more
@dblock
Copy link
Member

dblock commented Jul 20, 2016

I think we want this to be a plain Hash, and we want to provide a way to wrap that with either a Hashie::Mash class, HashWithIndifferentAccess or whatever else for backward compatibility. I'd take a PR that accomplishes that.

@dblock
Copy link
Member

dblock commented Oct 21, 2016

@etehtsea I am still very interested in this, want to give my comment a shot?

@nbulaj
Copy link
Contributor

nbulaj commented Oct 27, 2016 •
edited
Loading

I agree that Grape needs to delete Hashie dependency and work with any any class that maybe respond to to_h. @dblock what do you think about that (why not just a simple Hash)?

@dblock
Copy link
Member

dblock commented Oct 28, 2016

I'm with you @nbulaj, see above for more detailed comments about what I suggest the plan to move forward could be.

@etehtsea etehtsea closed this Oct 28, 2016
@etehtsea
Copy link
Contributor Author

@dblock I couldn't make it, so please reopen it as issue if needed.

@dblock
Copy link
Member

dblock commented Oct 28, 2016 •
edited
Loading

Lets leave this open, it's a valid feature request and a good start.

@dblock dblock reopened this Oct 28, 2016
@etehtsea
Copy link
Contributor Author

@dblock I meant that this PR won't be ever merged so it will be better to open issue that someone could assign to.

@dblock
Copy link
Member

dblock commented Oct 30, 2016

Lets leave this as is. Issues, PRs, all the same ;)

@dblock dblock mentioned this pull request Oct 30, 2016
Closed
@dblock dblock mentioned this pull request Feb 20, 2017
Closed
@dblock
Copy link
Member

dblock commented Apr 14, 2017

Closing via #1610.

@dblock dblock closed this Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@etehtsea @dblock @nbulaj