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

Fields doc #1808

Merged
merged 8 commits into from
Jun 24, 2016
Merged

Fields doc #1808

merged 8 commits into from
Jun 24, 2016

Conversation

luizkowalski
Copy link
Contributor

Added docs for fields param

See #1807

@groyoh
Copy link
Member

groyoh commented Jun 16, 2016

@luizkowalski thanks for this 👍 Could you add an entry to the CHANGELOG under the misc section?

@@ -73,7 +73,11 @@ See [ARCHITECTURE](../ARCHITECTURE.md) for more information.

#### fields

PR please :)
```ruby
render json: @user, fields: [:access_token]
Copy link
Member

Choose a reason for hiding this comment

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

This is only valid for the json and attributes adapter. For the json_api adapter, you would use

render json: @user, fields: { users: [:access_token] }

Where users is the JSONAPI type.

Could you add the missing code snippet with a brief description (stating for which adapter each snippet is valid) or add a description above your code that states that it only valid for json and attributes adapter?

@groyoh
Copy link
Member

groyoh commented Jun 16, 2016

@luizkowalski once you've updated the PR please squash your commits into a single one. Thanks again for spending the time on this! 💯

Added md file

Changelog

Better phrases
@luizkowalski
Copy link
Contributor Author

I guess thats it.
Thanks so much for the feedback, it was really helpful for next PRs 👍

end
```

Note that this is only valid for the `json` adapter. For the `json_api` adapter, you would use
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to add another comment (last one I promise 😄 ) but could you add that it is also valid for the attributes adapter (also in the other file) e.g.

-Note that this is only valid for the `json` adapter.
+Note that this is only valid for the `json` and `attributes` adapters.

This is very important as it may be confusing for people using the attributes adapter if it is omitted.

And kudos for your quick responsiveness!

Added md file

Changelog

Better phrases

Added info for other adapters
@luizkowalski
Copy link
Contributor Author

Just updated
Let me know if everything is right and thanks for the corrections!

For example, if you have a serializer like this

```ruby
class UserSerializer < ActiveModel::Serializer
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should begin recommending ApplicationSerializer...

@@ -17,6 +17,7 @@ Fixes:
Misc:
- [#1734](https://github.com/rails-api/active_model_serializers/pull/1734) Adds documentation for conditional attribute (@lambda2)
- [#1685](https://github.com/rails-api/active_model_serializers/pull/1685) Replace `IncludeTree` with `IncludeDirective` from the jsonapi gem.
- [#1808](https://github.com/rails-api/active_model_serializers/pull/1808) Adds documentation for `fields` option. (@luizkowalski)
Copy link
Member

Choose a reason for hiding this comment

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

This will need to go in the 'unreleased' section

Copy link
Member

Choose a reason for hiding this comment

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

@bf4 isn't it already the case? Or was it updated already?

Copy link
Member

Choose a reason for hiding this comment

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

rebase against master will show this is not in 0.10.1

Copy link
Member

Choose a reason for hiding this comment

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

Oh true!

Copy link
Member

Choose a reason for hiding this comment

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

@luizkowalski could you rebased and update the changelog please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to rebase from master but it says its up to date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, I didn't understood what should be changed on changelog, sorry. this is becoming confusing

Copy link
Member

Choose a reason for hiding this comment

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

The latest master has a v0.10.1 section. The version of the CHANGELOG in your PR has the newest release section as v0.10.0 (2016-05-17). I believe other maintainers are asking that you rebase from rails-api/active_model_serializers master to get the latest CHANGELOG and ensure your CHANGELOG entry remains under the master (unreleased) section.

@groyoh groyoh merged commit d27b21a into rails-api:master Jun 24, 2016
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

4 participants