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

Improve doc for :root option in as_json() #36092

Merged
merged 1 commit into from May 13, 2019
Merged

Conversation

imechemi
Copy link
Contributor

:root option is much more pleasant than I'd imagined.

@@ -42,6 +42,13 @@ module JSON
# # => { "user" => { "id" => 1, "name" => "Konata Izumi", "age" => 16,
# # "created_at" => "2006-08-01T17:27:13.000Z", "awesome" => true } }
#
# If you prefer to use custom name than the object's type, set <tt>:root</tt> to the custom name as in:
Copy link
Member

Choose a reason for hiding this comment

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

I would say If you prefer, <tt>:root</tt> may also be set to a custom string key: instead. as_json(root: :author) would make the root a symbol which I don't think we want. This might also signal room for improvement in the API since we are expecting keys to be strings here, and not calling to_s on custom roots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded d8c5b08

as_json(root: :author) would make the root a symbol which I don't think we want.

It makes root a string key, not symbol.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't call root.to_s on this line so calling user.as_json(root: :author) would result in a symbol root key. Unless I'm misunderstanding the intent of this patch, please update the example code to use a string.

Also, please squash you commits into one, and then I'll merge this. Thanks! 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmcgibbon I was just deceived by what render json was doing. But I was able to see what you were saying. My bad.

I have made the change and squashed my commit. Hope it's good now.

@imechemi
Copy link
Contributor Author

Guess I triggered CI build, can someone stop it plz. Thanks

Remove trailing whitespace [ci skip]

Reword

Root value should be string [ci skip]
Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

👍

@gmcgibbon gmcgibbon merged commit 93ef756 into rails:master May 13, 2019
@imechemi imechemi deleted the update-doc branch May 13, 2019 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants