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

Use IndifferentHash class instead of method #317

Merged
merged 2 commits into from
Nov 21, 2017
Merged

Use IndifferentHash class instead of method #317

merged 2 commits into from
Nov 21, 2017

Conversation

dmathieu
Copy link
Contributor

@dmathieu dmathieu commented Nov 8, 2017

The indifferent_params method doesn't exist anymore in Sinatra 2.0 (See sinatra/sinatra@44c3cb8#diff-f2bc16b264ca20ca842cf15d82960d34 ).

cc @bigkevmcd

The indifferent_params method doesn't exist anymore in Sinatra 2.0 (See sinatra/sinatra@44c3cb8#diff-f2bc16b264ca20ca842cf15d82960d34 ).
@brandur
Copy link
Member

brandur commented Nov 8, 2017

Thanks for the tests! This looks good to me (also, this makes way more sense as a class method so I'm relatively glad to see that Sinatra changed it even if it's annoying).

Do you know if @gudmundur or someone else can run a release? I haven't done one in a while.

@gudmundur
Copy link
Member

Thanks @dmathieu. Will this work with Sinatra 1? I know the class exists, but I'm not sure it'll behave the same way.

If this doesn't work with Sinatra 1, we can either:

  1. Add a support method that'll detect whether the method or class should be used. Thus support both versions.
  2. Drop support for Sinatra 1, and cut a major version of Pliny.

My personal preference would be to support both, with a comment about dropping some code in the future once we go Sinatra 2 only.

We want to keep Sinatra 1 compatibility for now
@dmathieu
Copy link
Contributor Author

dmathieu commented Nov 9, 2017

Very good point. I've just added support for both.

@gudmundur
Copy link
Member

@dmathieu Nice work. This looks good to me. @brandur does this seem solid to you as well?

@brandur
Copy link
Member

brandur commented Nov 16, 2017

Yeah, looks good to me! Thanks @dmathieu, and nice catch on that one @gudmundur.

@gudmundur gudmundur merged commit 51ee665 into interagent:master Nov 21, 2017
@dmathieu dmathieu deleted the indifferent-hash branch November 21, 2017 13:22
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.

3 participants