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

fix-serializable-hash-option #219

Merged
merged 5 commits into from
Jun 20, 2016

Conversation

sbatykov
Copy link
Contributor

@sbatykov sbatykov commented May 4, 2016

No description provided.

@dblock
Copy link
Member

dblock commented May 4, 2016

I am not sure what this does, it needs specs and a CHANGELOG entry please?

@sbatykov
Copy link
Contributor Author

sbatykov commented May 6, 2016

       def serializable_value(entity, options)
          partial_output = valid_value(entity, options)
          if partial_output.respond_to?(:serializable_hash)
            partial_output.serializable_hash
          elsif partial_output.is_a?(Array) && partial_output.all? { |o| o.respond_to?(:serializable_hash) }
            partial_output.map(&:serializable_hash)
          elsif partial_output.is_a?(Hash)
            partial_output.each do |key, value|
              partial_output[key] = value.serializable_hash if value.respond_to?(:serializable_hash)
            end
          else
            partial_output
          end
        end

Options already pass to output result in partial_output = valid_value(entity, options) and can be modified self-own way.
When call partial_output.serializable_hash(option) options will be merged.

Also all others else branch, didn't pass options to serializable_hash. May be it's old code.

I will add CHANGLOG entry.

@dblock
Copy link
Member

dblock commented May 6, 2016

Ok makes sense. You need to write a test that shows a failure without this fix, please.

Batykov Stepan added 2 commits May 13, 2016 13:21
Add changelog
@sbatykov
Copy link
Contributor Author

commit test and change log

let(:fresh_class) { Class.new(Grape::Entity) }

it 'Test' do
class Address < Grape::Entity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are global scope. Can you declare them with let(:address) { Class.new(...) } please?

@dblock
Copy link
Member

dblock commented May 13, 2016

Thanks. See my comments on tests. Squash your commits too please if you can.

@dblock dblock merged commit 8f5b029 into ruby-grape:master Jun 20, 2016
@dblock
Copy link
Member

dblock commented Jun 20, 2016

Merged.

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.

2 participants