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

Add interpolation value on messages #1322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JuarezLustosa
Copy link

This pr fix interpolation and case value are added to the message, like this. with_message('the value: %{value} is not inside the acceptable list')

Fix this or #1233

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This is a good try, but I am not sure that this is testing everything that needs to be tested. This scenario for the test you updated seems to be that the model has a custom validation that happens to mimic what validates_inclusion_of. I don't think it's necessary to reproduce that in fixing #1233; I think you can assume the model has validates_inclusion_of somewhere in it.

A better place to test this bugfix would probably be in both of the shared contexts that are being utilized in this file ("it supports in_array" and "it supports in_range"), since someone will encounter this bug when using:

validates_inclusion_of :rating, in: [1, 3, 5], message: 'the value: %{value} is not 1, 3, or 5'

just as much as:

validates_inclusion_of :rating, in: 1..5, message: 'the value: %{value} is not between 1 and 5'

Secondly, we will probably want to make sure that there is a test (or multiple tests) that make use of with_message, with_low_message and with_high_message, and not just with_low_message and with_high_message.

I realize there are a LOT of tests for validate_inclusion_of, so feel free to ask more questions if you are confused.

@vsppedro
Copy link
Collaborator

Hi @JuarezLustosa, hope you are ok.

Do you still intend to work on this PR?

I was thinking about adding more tests to this PR.

Thank you for your contribution.

@JuarezLustosa
Copy link
Author

Hi @JuarezLustosa, hope you are ok.

Do you still intend to work on this PR?

I was thinking about adding more tests to this PR.

Thank you for your contribution.

Hey @vsppedro. All good, hope the same.

Go ahead, You can it. Thank you 😃

@vsppedro vsppedro self-assigned this Jul 10, 2021
message.gsub(/%{value}/, value.to_s)
end

def menssage_constructor(string, value)
Copy link

@varunlalan varunlalan Jan 4, 2022

Choose a reason for hiding this comment

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

typo message_constructor

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

Successfully merging this pull request may close these issues.

4 participants