-
Notifications
You must be signed in to change notification settings - Fork 110
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
Feature proposal: Honor all conditional checks config - path for sparse fields #308
Changes from all commits
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 | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,25 +18,49 @@ def extract(object, local_options) | |||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def skip?(field_name, object, local_options) | ||||||||||||||||||||||||||||
return true if if_callable && !if_callable.call(field_name, object, local_options) | ||||||||||||||||||||||||||||
if Blueprinter.configuration.enforce_all_conditions | ||||||||||||||||||||||||||||
any_global_or_field_condition_failed?(field_name, object, local_options) | ||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||
return true if if_callable && !if_callable.call(field_name, object, local_options) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
unless_callable && unless_callable.call(field_name, object, local_options) | ||||||||||||||||||||||||||||
unless_callable && unless_callable.call(field_name, object, local_options) | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
private | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
alejandroereyes marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||
def any_global_or_field_condition_failed?(field_name, object, local_options) | ||||||||||||||||||||||||||||
return true if if_callables.any? { |if_call| !if_call.call(field_name, object, local_options) } | ||||||||||||||||||||||||||||
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. This is not very DRY. It is repeating the same logic as the original |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
unless_callables.any? { |unless_call| unless_call.call(field_name, object, local_options) } | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def if_callable | ||||||||||||||||||||||||||||
return @if_callable if defined?(@if_callable) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
@if_callable = callable_from(:if) | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def if_callables | ||||||||||||||||||||||||||||
[ | ||||||||||||||||||||||||||||
extract_callable_from(Blueprinter.configuration.if), | ||||||||||||||||||||||||||||
extract_callable_from(options[:if]) | ||||||||||||||||||||||||||||
].select { |callable| callable } | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
Comment on lines
+44
to
+50
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. The following modification should allow this method to be used for both the legacy conditions, and the case where
Suggested change
delegate :enforce_all_conditions, to: Blueprinter.configuration |
||||||||||||||||||||||||||||
def unless_callable | ||||||||||||||||||||||||||||
return @unless_callable if defined?(@unless_callable) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
@unless_callable = callable_from(:unless) | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def unless_callables | ||||||||||||||||||||||||||||
[ | ||||||||||||||||||||||||||||
extract_callable_from(Blueprinter.configuration.unless), | ||||||||||||||||||||||||||||
extract_callable_from(options[:unless]) | ||||||||||||||||||||||||||||
].select { |callable| callable } | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
Comment on lines
+57
to
+62
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. The following modification should allow this method to be used for both the legacy conditions, and the case where
Suggested change
delegate :enforce_all_conditions, to: Blueprinter.configuration |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def callable_from(condition) | ||||||||||||||||||||||||||||
config = Blueprinter.configuration | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -47,13 +71,17 @@ def callable_from(condition) | |||||||||||||||||||||||||||
config.public_send(condition) | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return false unless tmp | ||||||||||||||||||||||||||||
extract_callable_from(tmp) | ||||||||||||||||||||||||||||
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. This whole method can go away if all condition gathering is moved to Or, you could refactor it to really DRY things up by pulling in the suggested logic from those two methods, and make this one the the do-everything method. That does have the potential to add too much complexity here, though, which is why I leaned in on the explicitly named methods that you created. |
||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
def extract_callable_from(tmp_callable) | ||||||||||||||||||||||||||||
return false unless tmp_callable | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
case tmp | ||||||||||||||||||||||||||||
when Proc then tmp | ||||||||||||||||||||||||||||
when Symbol then blueprint.method(tmp) | ||||||||||||||||||||||||||||
case tmp_callable | ||||||||||||||||||||||||||||
when Proc then tmp_callable | ||||||||||||||||||||||||||||
when Symbol then blueprint.method(tmp_callable) | ||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||
raise ArgumentError, "#{tmp.class} is passed to :#{condition}" | ||||||||||||||||||||||||||||
raise ArgumentError, "#{tmp_callable.class} is passed to :#{condition}" | ||||||||||||||||||||||||||||
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. I think you missed adding a test for this error case, because This extraction method is nice, though. It would be nice to keep it. A better solution, I think, would be to
That way you get errors sooner and with a more meaningful stack trace. |
||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -333,6 +333,43 @@ def self.unless_method(_field_name, _object, _options) | |||||
include_examples 'does not serialize the conditional field' | ||||||
end | ||||||
end | ||||||
|
||||||
context 'enforce_all_conditions is enabled' do | ||||||
let(:local_options) { {x: 1, y: 2, v1: value, v2: other_value} } | ||||||
|
||||||
before do | ||||||
Blueprinter.configuration.enforce_all_conditions = true | ||||||
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. Setting and resetting global configurations for testing is not an ideal practice.
Suggested change
and then, as a bonus, you don't need the |
||||||
Blueprinter.configuration.if = ->(field_name, object, local_opts) { | ||||||
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. Ideally this could also be stubbed with an |
||||||
if local_opts[:sparse_fields] | ||||||
local_opts[:sparse_fields].include?(field_name.to_s) | ||||||
else | ||||||
value | ||||||
end | ||||||
} | ||||||
Blueprinter.configuration.unless = ->(_a,_b,_c) { other_value } | ||||||
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. ditto |
||||||
end | ||||||
after do | ||||||
Blueprinter.configuration.enforce_all_conditions = false | ||||||
Blueprinter.configuration.if = nil | ||||||
Blueprinter.configuration.unless = nil | ||||||
end | ||||||
|
||||||
if value && !other_value | ||||||
include_examples 'serializes the conditional field' | ||||||
|
||||||
context 'and top level conditional triggered by local options' do | ||||||
let(:local_options) { {sparse_fields: ['first_name']} } | ||||||
|
||||||
it 'honors all configuration conditionals - only serializes specified field' do | ||||||
should eq(%({"first_name":"Meg"})) | ||||||
end | ||||||
end | ||||||
else | ||||||
it 'nothing is serialized' do | ||||||
should eq('{}') | ||||||
end | ||||||
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.
"true if this and not this" just spins my head a little. The following suggestion is to replace the entire
skip?
method, though:Basically, I might make
skip?
work a little more like Railsblank?
andpresent?
for readability (but in this case,skip?
is!include?
)