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

Read message value directly from options #141

Merged
merged 1 commit into from May 1, 2016

Conversation

monfresh
Copy link
Contributor

@monfresh monfresh commented Apr 28, 2016

Why:
With the changes in PR #139, specifying a symbolized value for the
message option would cause that method to be called on the Model
object. If that method didn't exist, it resulted in a NoMethodError
exception. If that method did exist in the Model, and also as an i18n
key, then the Model method was used, as opposed to the i18n key.

The message option is meant to either be a String, or a symbol
corresponding to an i18n key. Therefore, it should be read directly
from options[:message] and not sent for further processing by the
options_value method.

How:

Closes #140.

@coveralls
Copy link

coveralls commented Apr 28, 2016

Coverage Status

Coverage decreased (-0.2%) to 99.796% when pulling b9ee211 on monfresh:fix/140-options-value into e22bc20 on joost:master.

**Why**:
With the changes in PR joost#139, specifying a symbolized value for the
`message` option would cause that method to be called on the Model
object. If that method didn't exist, it resulted in a `NoMethodError`
exception. If that method did exist in the Model, and also as an i18n
key, then the Model method was used, as opposed to the i18n key.

The `message` option is meant to either be a String, or a symbol
corresponding to an i18n key. Therefore, it should be read directly
from `options[:message]` and not sent for further processing by the
`options_value` method.

**How**:
- Revert the `error_message` method back to its value prior to PR joost#139,
so that it uses `options[:message]`
- Add 3 new specs to cover these scenarios
@coveralls
Copy link

coveralls commented Apr 28, 2016

Coverage Status

Coverage decreased (-0.2%) to 99.796% when pulling d3b9f10 on monfresh:fix/140-options-value into e22bc20 on joost:master.

@monfresh
Copy link
Contributor Author

For some reason, Coveralls is including the spec file, which is why it says the coverage has decreased. I've had similar issues in the past with Coveralls. I recommend using Code Climate instead. It provides static analysis as well as test coverage.

@monfresh
Copy link
Contributor Author

@joost When you get a chance, it would be great to have this reviewed and merged. Thanks!

@joost joost merged commit ceadb46 into joost:master May 1, 2016
@monfresh
Copy link
Contributor Author

monfresh commented May 3, 2016

Thanks! When can we expect a new version to be released?

@joost
Copy link
Owner

joost commented May 8, 2016

Bumped version to 0.14.1.

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.

None yet

3 participants