Prevent creation of unnecessary fieldset(mirror and revised)#2450
Prevent creation of unnecessary fieldset(mirror and revised)#2450liijunwei wants to merge 1 commit intorails-api:0-10-stablefrom
Conversation
| next if resource_object.nil? | ||
| # NOTE(BF): the attributes are cached above, separately from the relationships, below. | ||
| requested_associations = fieldset.fields_for(resource_object[:type]) || '*' | ||
| requested_fields = fieldset && fieldset.fields_for(resource_object[:type]) |
There was a problem hiding this comment.
change#2: only ask for fields_for when fieldset is not nil
this is not a change, it just saves the fields_for cost
also align with above
liijunwei
force-pushed
the
prevent_creation_of_unnecessary_fieldset
branch
2 times, most recently
from
aa20faa to
1af09b5
Compare
|
hello @bf4 , sorry about pinging directly, could you take a look when you have a moment ? 🙏 |
|
I'm on vacation and haven't read too carefully If tests pass and it works for people, that's fine with me There are some changes which are refactors which may introduce possible bugs. When that's the case, it's often worth extracting just the refactors and any work which supports the change you want, but doesn't change behavior, so that the actual code change which changes the behavior is smaller and safer and easier to review. That is, make a new pr without the behavior changes, merge it, then rebase/update the feature pr |
|
ok, let me separate the change from refactoring have a nice vacation :D |
liijunwei
force-pushed
the
prevent_creation_of_unnecessary_fieldset
branch
2 times, most recently
from
26a1dd8 to
c34dc87
Compare
| super | ||
| instance_options[:fieldset] ||= ActiveModel::Serializer::Fieldset.new(fields_to_fieldset(instance_options.delete(:fields))) | ||
|
|
||
| instance_options[:fieldset] ||= begin |
There was a problem hiding this comment.
change#1: only initialize ActiveModel::Serializer::Fieldset when fieldset is not nil
instance_options[:fieldset] could be:
- the
fieldsetpassed in through option ActiveModel::Serializer::Fieldsetinstance- nil
There was a problem hiding this comment.
That's fine. Or add a class method on fieldset which does what's in this begin block
There was a problem hiding this comment.
echoing what you previously mentioned, I also want keep the changes minimal 🙏
if it's necessary, is it better to do it in another refactoring PR?
| super | ||
| @include_directive = JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true) | ||
| @fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options.delete(:fields)) | ||
| @fieldset = options[:fieldset] || ((option_fields = options.delete(:fields)) && ActiveModel::Serializer::Fieldset.new(option_fields)) |
There was a problem hiding this comment.
@fieldset could be:
- the
fieldsetpassed in through option ActiveModel::Serializer::Fieldsetinstance- nil
|
Updated: only include desired changes for performance optimization |
liijunwei
force-pushed
the
prevent_creation_of_unnecessary_fieldset
branch
2 times, most recently
from
40befea to
73420cd
Compare
liijunwei
force-pushed
the
prevent_creation_of_unnecessary_fieldset
branch
from
73420cd to
a47c639
Compare
|
hello @bf4 , sorry to bother , just would like to see whether the changes looks good to you ? Orz |
|
Seems fine |
2 participants
Purpose
see: #2370
Changes
ActiveModel::Serializer::Fieldsetwhen fieldset is not nilchange#2: only ask forfields_forwhenfieldsetis not nilCaveats
NA
Related GitHub issues
Additional helpful information