fix blank error message when allow_blank is false and value is blank #934
Conversation
| @@ -50,6 +50,16 @@ def self.raise_if_missing_params | |||
|
|
|||
| # check if value is valid | |||
| def valid?(value) | |||
| if param_description.is_a?(Apipie::ParamDescription) | |||
| if (param_description.options[:allow_nil] == false) && value.nil? | |||
There was a problem hiding this comment.
pass when param_description.options[:allow_nil] value is nil, only false
| @@ -305,7 +305,7 @@ | |||
| let(:validation_value) { '' } | |||
|
|
|||
| it 'raises an error' do | |||
| expect { subject }.to raise_error(Apipie::ParamInvalid) | |||
| expect { subject }.not_to raise_error(Apipie::ParamInvalid) | |||
There was a problem hiding this comment.
i think this test code is wrong or 188row test code is wrong or my bad?
There was a problem hiding this comment.
if it used to raise an error but no longer, there must be a reason.
if that is the new expectation, we should update the test title
There was a problem hiding this comment.
when using not_to raise_error we should remove the exception class.
because a different expectation might be raised instead here, creating a false positive
| @@ -50,6 +50,16 @@ def self.raise_if_missing_params | |||
|
|
|||
| # check if value is valid | |||
| def valid?(value) | |||
| if param_description.is_a?(Apipie::ParamDescription) | |||
There was a problem hiding this comment.
some time param_description to be "" in test_code
I'm not sure there's a case like that.
But it's a precaution
i will fix if test code is bad
pekopekopekopayo
force-pushed
the
fix/validate_blank_error_message
branch
from
68fae8f
to
fe66db0
Compare
pekopekopekopayo
force-pushed
the
fix/validate_blank_error_message
branch
from
700b051
to
fe66db0
Compare
mathieujobin
changed the title
| @@ -1,4 +1,8 @@ | |||
| class CustomBoolValidator < Apipie::Validator::BaseValidator | |||
| def valid?(value) | |||
| validate(value) | |||
| end | |||
There was a problem hiding this comment.
method alias or do without entirely?
| @@ -480,6 +490,9 @@ def self.validate(value) | |||
| end | |||
|
|
|||
| class BooleanValidator < BaseValidator | |||
| def valid?(value) | |||
| validate(value) | |||
| end | |||
There was a problem hiding this comment.
much value putting this in multiple validator ?
| @@ -120,11 +120,11 @@ def validate(value) | |||
| return true if allow_nil && value.nil? | |||
| return true if allow_blank && value.blank? | |||
| value = normalized_value(value) | |||
| if (!allow_nil && value.nil?) || (blank_forbidden? && value.blank?) || !validator.valid?(value) | |||
There was a problem hiding this comment.
I don't know if we can remove the !allow_nil check and the blank_forbidden? check.
I think they were added for a reason...
I don't understand what problem we are trying to fix here.
|
closing for now, reopen if you think there is still value. sorry if its just me who don't understand. |
#933
param blank validator error message was wrong and i will fix