Skip to content
This repository has been archived by the owner on Feb 7, 2018. It is now read-only.

Add error when attribute is present but value is nil and has no default #78

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tomdev
Copy link
Member

@tomdev tomdev commented Oct 23, 2013

No description provided.

@@ -51,13 +51,15 @@ def raise_unauthorized(message = 'Unauthorized')
end

def check_validation
fail Pavlov::ValidationError, "Missing arguments: #{missing_arguments.inspect}" if missing_arguments.any?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want a line in UPGRADING to inform people that we will no longer raise this exact exception.

!attribute.options.key?(:default) && send(attribute.name).nil?
def check_missing_arguments
attribute_set.each do |attribute|
if !attribute.options.key?(:default) && send(attribute.name).nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there a way which checks whether the argument was passed in? This way you cannot pass in nil anywhere (except when this is an explicit default). That's no missing argument. We might want to check for nils as well, but I'm not totally sure. If there is no other way to check missing arguments this is fine though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I originally wrote this code I couldn't find any way, but maybe the newer Virtus has more support for this. If nothing else, we could hook into the initializer now, and set all non-passed-in attributes to a AttributeNotGiven constant or something and then check for that here.

@marten
Copy link
Contributor

marten commented Oct 29, 2013

@tomdev what's happening with this PR?

@jjoos
Copy link
Member

jjoos commented Oct 31, 2013

@tomdev Could you check this out?

@tomdev
Copy link
Member Author

tomdev commented Oct 31, 2013

It still doesn't look like Virtus supports checking missing arguments. I like @marten his suggestion, but don't have time to work on it right now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants