Skip to content
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

fix blank error message when allow_blank is false and value is blank #934

Conversation

pekopekopekopayo
Copy link
Contributor

@pekopekopekopayo pekopekopekopayo commented Jun 4, 2024

#933

param blank validator error message was wrong and i will fix

@pekopekopekopayo pekopekopekopayo marked this pull request as draft June 4, 2024 16:08
@@ -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?
Copy link
Contributor Author

@pekopekopekopayo pekopekopekopayo Jun 4, 2024

Choose a reason for hiding this comment

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

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this test code is wrong or 188row test code is wrong or my bad?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@pekopekopekopayo pekopekopekopayo marked this pull request as ready for review June 4, 2024 17:29
@@ -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)
Copy link
Contributor Author

@pekopekopekopayo pekopekopekopayo Jun 4, 2024

Choose a reason for hiding this comment

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

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 pekopekopekopayo force-pushed the fix/validate_blank_error_message branch from 68fae8f to fe66db0 Compare June 13, 2024 15:47
@pekopekopekopayo pekopekopekopayo marked this pull request as draft June 13, 2024 16:02
@pekopekopekopayo pekopekopekopayo force-pushed the fix/validate_blank_error_message branch from 700b051 to fe66db0 Compare June 13, 2024 16:17
@mathieujobin mathieujobin changed the title fix blank error message when allow_blank is false and value is balnk fix blank error message when allow_blank is false and value is blank Jul 3, 2024
@@ -1,4 +1,8 @@
class CustomBoolValidator < Apipie::Validator::BaseValidator
def valid?(value)
validate(value)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

method alias or do without entirely?

@@ -480,6 +490,9 @@ def self.validate(value)
end

class BooleanValidator < BaseValidator
def valid?(value)
validate(value)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@mathieujobin
Copy link
Collaborator

closing for now, reopen if you think there is still value.
I think new tests should be added and existing test should be passing, unless the change makes sense.

sorry if its just me who don't understand.

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.

2 participants