-
Notifications
You must be signed in to change notification settings - Fork 420
Support automatic pluralization of types #376
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
base: dev
Are you sure you want to change the base?
Changes from all commits
ee76e0c
fdcaed6
a160d67
e0228da
bc04aee
d81f8bb
b13cc36
22e2b2c
4a0ef07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
module FastJsonapi | ||
VERSION = "1.4" | ||
VERSION = "1.5" | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -486,4 +486,63 @@ def year_since_release_calculator(release_year) | |
end | ||
end | ||
end | ||
|
||
describe '#pluralize_type' do | ||
subject(:serializable_hash) { MovieSerializer.new(movie).serializable_hash } | ||
|
||
before do | ||
MovieSerializer.pluralize_type pluralize | ||
end | ||
|
||
after do | ||
MovieSerializer.pluralize_type nil | ||
MovieSerializer.set_type :movie | ||
end | ||
|
||
context 'when pluralize is true' do | ||
let(:pluralize) { true } | ||
|
||
it 'returns correct hash which type equals pluralized value' do | ||
expect(serializable_hash[:data][:type]).to eq :movies | ||
end | ||
end | ||
|
||
context 'when pluralize is false' do | ||
let(:pluralize) { false } | ||
|
||
it 'returns correct hash which type equals non-pluralized value' do | ||
expect(serializable_hash[:data][:type]).to eq :movie | ||
end | ||
end | ||
end | ||
|
||
describe '#pluralize_type after #set_type' do | ||
subject(:serializable_hash) { MovieSerializer.new(movie).serializable_hash } | ||
|
||
before do | ||
MovieSerializer.set_type type_name | ||
MovieSerializer.pluralize_type true | ||
end | ||
|
||
after do | ||
MovieSerializer.pluralize_type nil | ||
MovieSerializer.set_type :movie | ||
end | ||
|
||
context 'when sets singular type name' do | ||
let(:type_name) { :film } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every example in this group should be using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain more about why this should be the case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Look at line 541. That's directly setting the type to be plural in the before block, before the Given that, both examples should be given a singular type name for these tests to check anything useful. In fact, you could even get rid of the let altogether and just say |
||
|
||
it 'returns correct hash which type equals transformed set_type value' do | ||
expect(serializable_hash[:data][:type]).to eq :films | ||
end | ||
end | ||
|
||
context 'when sets plural type name' do | ||
let(:type_name) { :films } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should remove this line, or else |
||
|
||
it 'returns correct hash which type equals transformed set_type value' do | ||
expect(serializable_hash[:data][:type]).to eq :films | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it stands there's an issue where type is not correctly pluralized in relationships. The following test shows this:
This also means there's a mismatch between relationship and include types. Currently:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, as far as I understand,
fast_jsonapi
does not actually "know" internally what serializer is associated with any given relationship. The relationship data is built via simply reflecting on the object and constructing the abbreviated relationship structure. In theory it's possible to make it aware of the serializer, but see my original "Additional Note" -- I ended up being worried about missing performance targets, so I didn't implement a change here.In order to make this scenario work, I implemented the ability to mark a specific relationship as plural with
pluralize_type: true
. (Check outobject_serializer_pluralization_spec.rb
.) This isn't ideal, but it solves this scenario in lieu of a higher-level decision about perf targets.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sort of glossed over that note when I first reviewed this PR, and after banging my head against this for a bit, I'm coming to the same conclusion... I'm going to investigate this a bit more because it is important for my use case, but I understand that the complexity involved in such a refactor may not be worth the effort.
At any rate it may not be worth including in this PR, as it already achieves the majority of what it sets out to do.