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

Consider evaluating association in serializer context #1378

Merged
merged 1 commit into from
Jan 4, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Dec 16, 2015

For discussion:

Consider evaluating association in serializer context

That way, associations are really just anything that
can be conditionally included. They no longer
have to actually be methods on the object or serializer.

e.g.

has_many :comments do
- last(1)
+ Comment.active.for_serialization(object).last(1)
end

ref: #1356

@beauby
Copy link
Contributor

beauby commented Dec 20, 2015

This is a must IMO. The current behavior is confusing.

@bf4 bf4 force-pushed the consider_association_blocks branch 2 times, most recently from 019b386 to c9c7666 Compare December 23, 2015 03:41
@beauby
Copy link
Contributor

beauby commented Dec 27, 2015

We should merge this. How should we go about warning people migrating from previous versions?

@bf4
Copy link
Member Author

bf4 commented Dec 27, 2015

Changelog? Update doc?

B mobile phone

On Dec 27, 2015, at 4:16 PM, Lucas Hosseini notifications@github.com wrote:

We should merge this. How should we go about warning people migrating from previous versions?


Reply to this email directly or view it on GitHub.

For discussion:

Consider evaluating association in serializer context

That way, associations are really just anything that
can be conditionally included.  They no longer
have to actually be methods on the object or serializer.

e.g.

```diff
has_many :comments do
- last(1)
+ Comment.active.for_serialization(object).last(1)
end
```
@bf4 bf4 force-pushed the consider_association_blocks branch from c9c7666 to d7de53c Compare December 30, 2015 04:16
@bf4
Copy link
Member Author

bf4 commented Dec 30, 2015

* Syntax changes from e.g.
`has_many :titles do customers.pluck(:title) end` (in #1356) to
`has_many :titles do object.customers.pluck(:title) end`
- [#1356](https://github.com/rails-api/active_model_serializers/pull/1356) Add inline syntax for
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this belong here?

Copy link
Member Author

Choose a reason for hiding this comment

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

why wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because this PR is #1378 and not #1356

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh... They're just missing from changelog. How would you prefer i include them?

B mobile phone

On Dec 31, 2015, at 12:05 PM, Lucas Hosseini notifications@github.com wrote:

In CHANGELOG.md:

@@ -16,7 +16,22 @@ Breaking changes:

Features:

-- #1336 Added support for Grape >= 0.13, < 1.0
+- #1378 Change association blocks


Reply to this email directly or view it on GitHub.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it to my local master and merged locally so it's two commits, but not part of this PR merge commit

@bf4 bf4 merged commit d7de53c into rails-api:master Jan 4, 2016
@bf4 bf4 deleted the consider_association_blocks branch January 4, 2016 05:17
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.

2 participants