-
Notifications
You must be signed in to change notification settings - Fork 153
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
Add default value option #352
Add default value option #352
Conversation
a057d16
to
91dc6c9
Compare
return unless valid?(entity) | ||
|
||
output = value(entity, options) | ||
output.blank? && @default_value.present? ? @default_value : output |
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 am not a maintainer, but I stumbled across this PR while I was looking for a sane default ability for entities.
blank?
and present?
are Rails specific, so to keep Grape framework agnostic you might want to shuffle this around. In fact, the blank?
check replaces some valid options like false
and []
with the default value, which I would not expect.
output.blank? && @default_value.present? ? @default_value : output | |
output.nil? ? @default_value : output |
I'm not sure the .present?
check bought you anything anyway if we keep it strictly to a nil check.
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.
.blank?
and .present?
are part of ActiveSupport and this is included in the Grap gemspec, so it can be used without adding additional dependencies
but suggest to move the getting of the value into the same named method → value()
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.
@LeFnord not sure if I get this part:
but suggest to move the getting of the value into the same named method → value()
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.
@LeFnord Do you still consider the behavior of blank?
to be appropriate though? If I had a boolean attribute with a default of true
, then the above implementation will always return true
no matter what, as:
false.blank?
# => true
It seems to me like the only sane value to override with a default would be nil
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.
@dlinch … I'm only saying, that is no problem to use it, cause it is in place
and yeap would prefer your suggestion but with a check on blank
, instead only nil
output.blank? ? @default_value.presence : output
sorry @ahmednaguib for the late reply |
Description
I noticed there is no direct way of defining default value for
expose
when a field is returningnil
. Of course there is a way to do it via a block but would be a nice option to have it like this:expose :x, default: 0
I also saw there is an issue opened #327 asking for the feature so I made the pr maybe it will be useful for other folks.