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

Add code example for include option of AM::Serialization#serializable_hash #19864

Conversation

radanskoric
Copy link
Contributor

ActiveModel::Serialization#serializable_hash code examples were not showcasing the includeoption at all so I extended the current example to include it.

# attr_accessor :city, :street
#
# def attributes
# {'city' => nil, 'street' => nil}
Copy link
Member

Choose a reason for hiding this comment

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

should not be city instead of nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not according to all the rest of the documentation in this class. All the other examples just use => nil everywhere.

In fact, the values are completely irrelevant. attributes is used in only one place and that is only to extract the keys.

As far as I can tell, the only reason a Hash is expected here instead of an array of keys is so it would have the same requirements as ActiveModel::AttributeMethods.

@radanskoric radanskoric force-pushed the add_includes_example_for_serialization branch from 673cb8b to 8091a3e Compare April 26, 2015 20:10
@radanskoric radanskoric force-pushed the add_includes_example_for_serialization branch from 8091a3e to efe3347 Compare June 9, 2015 20:29
@radanskoric radanskoric closed this Jun 9, 2015
@radanskoric radanskoric deleted the add_includes_example_for_serialization branch June 9, 2015 20:36
@rafaelfranca
Copy link
Member

Applied at 3d949f3. Thanks

@radanskoric
Copy link
Contributor Author

Hey, @rafaelfranca thanks, but I killed this PR because, while it was open, a different PR was opened and merged so there is now a longer explanation in place and this PR was no longer relevant: https://github.com/rails/rails/blob/master/activemodel/lib/active_model/serialization.rb#L111-L140

So what you merged is just some example preparation code without actual usage example (since I noticed the other commit only after I rebased and cleaned up the conflict) which makes it a bit confusing. So sadly I think this commit should be reverted. :(

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.

None yet

2 participants