Skip to content

Add default value option#352

Merged
LeFnord merged 2 commits intoruby-grape:masterfrom
ahmednaguib:add-default-value-option
Sep 15, 2021
Merged

Add default value option#352
LeFnord merged 2 commits intoruby-grape:masterfrom
ahmednaguib:add-default-value-option

Conversation

@ahmednaguib
Copy link
Copy Markdown
Contributor

Description

I noticed there is no direct way of defining default value for expose when a field is returning nil. 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.

@ahmednaguib ahmednaguib force-pushed the add-default-value-option branch from a057d16 to 91dc6c9 Compare June 11, 2021 09:34
return unless valid?(entity)

output = value(entity, options)
output.blank? && @default_value.present? ? @default_value : output
Copy link
Copy Markdown

@dlinch dlinch Jul 21, 2021

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.

Suggested change
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.

Copy link
Copy Markdown
Member

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()

Copy link
Copy Markdown
Contributor Author

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()

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

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

@LeFnord
Copy link
Copy Markdown
Member

LeFnord commented Jul 22, 2021

sorry @ahmednaguib for the late reply

@LeFnord LeFnord merged commit 2c37a87 into ruby-grape:master Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants