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.

Sign up for GitHub

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

Jump to bottom

Rename ArraySerializer to CollectionSerializer for clarity #1251

Merged
merged 1 commit into from
Oct 22, 2015
Merged

Rename ArraySerializer to CollectionSerializer for clarity #1251

merged 1 commit into from
Oct 22, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Oct 7, 2015

No description provided.

NullVoxPopuli
NullVoxPopuli reviewed Oct 7, 2015
View reviewed changes
lib/active_model/serializer/array_serializer.rb
class ActiveModel::Serializer
class ArraySerializer < CollectionSerializer
def initialize(*)
warn "Calling deprecated ArraySerializer in #{caller[0]}. Please use CollectionSerializer"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@beauby
Copy link
Contributor

beauby commented Oct 7, 2015

I have a feeling pagination won't work out of the box with this.

@beauby
Copy link
Contributor

beauby commented Oct 8, 2015

@bf4 Can you confirm this does not break handling of paginated collections as it previously was? I'm thinking of: https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/adapter/json_api.rb#L96

@NullVoxPopuli
Copy link
Contributor

I guess if there aren't tests for that, there should be. @beauby, could you
check on that?

On Thu, Oct 8, 2015, 6:39 PM Lucas Hosseini notifications@github.com
wrote:

@bf4 https://github.com/bf4 Can you confirm this does not break
handling of paginated collections as it previously was? I'm thinking of:
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/adapter/json_api.rb#L96

—
Reply to this email directly or view it on GitHub
#1251 (comment)
.

@beauby
Copy link
Contributor

beauby commented Oct 8, 2015

The problem is that support for pagination currently relies on external gems.

@NullVoxPopuli
Copy link
Contributor

Could we add a pagination gem just in the test environment?

On Thu, Oct 8, 2015, 7:17 PM Lucas Hosseini notifications@github.com
wrote:

The problem is that support for pagination currently relies on external
gems.

—
Reply to this email directly or view it on GitHub
#1251 (comment)
.

@beauby
Copy link
Contributor

beauby commented Oct 8, 2015

Better mock it up. Those usually rely on AR.

@bf4
Copy link
Member Author

bf4 commented Oct 8, 2015

How did you know this was broken? I didn't see anything. The gems are for sure used in tests and hence abailable, IIRC

B mobile phone

On Oct 8, 2015, at 6:20 PM, L. Preston Sego III notifications@github.com wrote:

Could we add a pagination gem just in the test environment?

On Thu, Oct 8, 2015, 7:17 PM Lucas Hosseini notifications@github.com
wrote:

The problem is that support for pagination currently relies on external
gems.

—
Reply to this email directly or view it on GitHub
#1251 (comment)
.

—
Reply to this email directly or view it on GitHub.

@beauby
Copy link
Contributor

beauby commented Oct 8, 2015

@bf4 I don't see the gems in the Gemfile, nor have I seen any test about pagination. Could you point me to those tests?

@bf4
Copy link
Member Author

bf4 commented Oct 8, 2015

I'll add an integration test but where are the failures? I recall it's green

B mobile phone

On Oct 8, 2015, at 6:21 PM, Lucas Hosseini notifications@github.com wrote:

Better mock it up. Those usually rely on AR.

—
Reply to this email directly or view it on GitHub.

@bf4
Copy link
Member Author

bf4 commented Oct 8, 2015

Gemspec

B mobile phone

On Oct 8, 2015, at 6:24 PM, Lucas Hosseini notifications@github.com wrote:

@bf4 I don't see the gems in the Gemfile, nor have I seen any test about pagination. Could you point me to those tests?

—
Reply to this email directly or view it on GitHub.

@beauby
Copy link
Contributor

beauby commented Oct 8, 2015

Wait, there have been some code changes since I posted that comment. Last I checked you got rid of the paginated? method on the Array/CollectionSerializer.

@bf4
Copy link
Member Author

bf4 commented Oct 9, 2015

If so not intentional. It's just a rename

B mobile phone

On Oct 8, 2015, at 6:27 PM, Lucas Hosseini notifications@github.com wrote:

Wait, there have been some code changes since I posted that comment. Last I checked you got rid of the paginated? method on the Array/CollectionSerializer.

—
Reply to this email directly or view it on GitHub.

@bf4 bf4 mentioned this pull request Oct 9, 2015
Merged
@bf4
Copy link
Member Author

bf4 commented Oct 9, 2015

@beauby could you clarify for me what is blocking this?

bf4
bf4 reviewed Oct 9, 2015
View reviewed changes
test/array_serializer_test.rb
def test_json_key_with_root_and_no_serializers
serializer = ArraySerializer.new(build_named_collection, root: 'custom_root')
assert_equal serializer.json_key, 'custom_roots'
class ArraySerializerTest < CollectionSerializerTest
Copy link
Member Author

Choose a reason for hiding this comment

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

for backwards compat https://github.com/rails-api/active_model_serializers/pull/1251/files#r41372642 also, is awesome

@beauby
Copy link
Contributor

beauby commented Oct 9, 2015

LGTM but needs rebase.

@bf4 bf4 force-pushed the collection_serializer branch from 7d979bc to 2c8b9b7 Compare October 21, 2015 21:57
bf4 added a commit that referenced this pull request Oct 22, 2015
@bf4
Merge pull request #1251 from bf4/collection_serializer
8e1245a 
Rename ArraySerializer to CollectionSerializer for clarity
@bf4 bf4 merged commit 8e1245a into rails-api:master Oct 22, 2015
@bf4 bf4 deleted the collection_serializer branch October 22, 2015 15:29
@NullVoxPopuli
Copy link
Contributor

👍

@bf4 bf4 mentioned this pull request Oct 26, 2015
Merged
2 tasks
@bf4 bf4 mentioned this pull request Nov 26, 2015
Merged
@bf4 bf4 mentioned this pull request Dec 4, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C: Cleanup S: Ready for PR V: 0.10.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bf4 @beauby @NullVoxPopuli