Skip to content

[WIP] Replace to_hash with as_json#32

Open
agrare wants to merge 1 commit into
ManageIQ:masterfrom
agrare:remove_to_hash_from_managed_object
Open

[WIP] Replace to_hash with as_json#32
agrare wants to merge 1 commit into
ManageIQ:masterfrom
agrare:remove_to_hash_from_managed_object

Conversation

@agrare

@agrare agrare commented Jan 6, 2023

Copy link
Copy Markdown
Member

Follow-up to https://github.com/ManageIQ/rbvmomi2/pull/29/files#r1062876312

@Fryguy I wonder if #as_json might be better since String#as_json returns a String just wanted to throw this up to start the coversation

@Fryguy

Fryguy commented Jan 6, 2023

Copy link
Copy Markdown
Member

How/where was to_hash being used previously? I understand it's for deserialization, but I'm not sure I fully understand how that particular method comes into play. I also don't understand the nuances of to_json vs as_json

@agrare

agrare commented Jan 6, 2023

Copy link
Copy Markdown
Member Author

How/where was to_hash being used previously

It was used for serializing these objects to json, there was a bug where .to_json would blow up serializing anything that included a ManagedObject (so basically everything). So we need a way to convert these objects to something that will to_json nicely, we haven't done this for .to_yaml because it didn't throw an exception but the idea was a common "to_hash" method that could to_json and to_yaml nicely.

@agrare

agrare commented Jan 6, 2023

Copy link
Copy Markdown
Member Author

I also don't understand the nuances of to_json vs as_json

I was thinking it would follow the rails as_json which returns a hash and then to_json which returns a json string (https://apidock.com/rails/ActiveModel/Serializers/JSON/as_json). You are right that to_hash returning a string is a little confusing which is why I thought as_json would be better, but then it doesn't work as well for going to to_yaml in the future.

@Fryguy

Fryguy commented Jan 6, 2023

Copy link
Copy Markdown
Member

Oh that's interesting - don't think I've seen as_json before, but it seems the benefit there is that it allows hiding things in ActiveModel via options. 🤔

@Fryguy Fryguy self-assigned this Jan 18, 2023
@Fryguy Fryguy added the bug Something isn't working label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants