-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Replace Hashie::Mash with ActiveSupport::HashWithIndifferentAccess #1514
Replace Hashie::Mash with ActiveSupport::HashWithIndifferentAccess #1514
Conversation
f4454d7
to
eb4276f
Compare
eb4276f
to
6545207
Compare
@@ -285,7 +285,7 @@ class User | |||
requires :file, type: Rack::Multipart::UploadedFile | |||
end | |||
subject.post '/upload' do | |||
params[:file].filename | |||
params[:file][:filename] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a change in API as only the top keys are with indifferent access?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope! to all sub keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dblock take a look here
[params[:a]['b'][:c], params['a'][:d]]
https://github.com/msroot/grape/blob/e36710635ed905c35af51d82b72d096c99503d39/spec/grape/endpoint_spec.rb#L263-L279
This is similar to #1448, but better. I think we also need to provide a backward compatible way for existing users to return a class API < Grape::API
# params is a regular Hash
end class API < Grape::API
include Grape::Extensions::...::Hashie::Mash
# params is Hashie::Mash
end class API < Grape::API
include Grape::Extensions::...::ActiveSupport::HashWithIndifferentAccess
# params is HashWithIndifferentAccess
end You get the idea ... |
@dblock so the default should be Does the user has to explicitly require the |
Yes, I think the default should be The user should explicitly include |
@dblock any timeline on merging this? |
This PR won't be merged without the changes I'm asking for above. |
Replaced with #1594. |
Please see #1610 as a working end-to-end PR. |
This will replace
Hashie::Mash
withActiveSupport::HashWithIndifferentAccess