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

renaming validate_numeric args for clarity. #867

Merged
merged 2 commits into from
Sep 8, 2022
Merged

renaming validate_numeric args for clarity. #867

merged 2 commits into from
Sep 8, 2022

Conversation

jwoertink
Copy link
Member

Fixes #824

This is a breaking change. The idea here is that if you had this code:

number = attribute(4)

validate_numeric number, greater_than: 4

To me, this reads like if 4 > 4 which is false, so you might expect this to return false, but it does not. It returns true because it runs if 4 >= 4. This especially gets weird if you're expecting it to be > and you put something like

validate_numeric age, greater_than: 17

Which would allow age == 17. The same is true on the upper end with less_than. less_than: 100 still allows 100.

Now these are renamed to at_least and no_more_than so you can say

validate_numeric age, at_least: 18, no_more_than: 99

@jwoertink jwoertink added the BREAKING CHANGE This will cause a breaking change label Aug 7, 2022
Copy link
Member

@matthewmcgarvey matthewmcgarvey left a comment

Choose a reason for hiding this comment

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

Can we do a deprecation cycle on this rather than breaking?

@jwoertink
Copy link
Member Author

Sure, though, this next release will be filled with breaking changes already... I figured we might as well get as many in now as we can so that way moving to 1.0 there's none. But I'm ok with doing a deprecation here 👍

@jwoertink
Copy link
Member Author

Added in the deprecation. This is what it'll look like.

In spec/avram/validations_spec.cr:346:35

 346 | result = Avram::Validations.validate_numeric(too_small_attribute, greater_than: 2)
                                   ^---------------
Warning: Deprecated Avram::Validations.validate_numeric:greater_than. Use validate_numeric with at_least/no_more_than instead of greater_than/less_than

@jwoertink jwoertink removed the BREAKING CHANGE This will cause a breaking change label Aug 21, 2022
@paulcsmith
Copy link
Member

Sorry to chime in late! I think at_least/no_more_than is more accurate than the incorrect greater_than, but may still not be the most clear.

What if we changed the name to exactly what it does using more "mathy" terms:

  • greater_than_or_equal_to
  • less_than_or_equal_to

And I'm not sure there is an option, but may be worth adding a greater_than and less_than that actually use > and < similar to https://guides.rubyonrails.org/active_record_validations.html#numericality

I think since these terms are fairly common and have a strictly defined meaning that may be more intuitive and accurate. It is more wordy for sure, but these types of validations typically aren't used that much, and if they are I think the wordiness is helpful to make sure it is accurate. What do you think?

@jwoertink
Copy link
Member Author

Sure! This is why I did rc1 and not a full release 😂 get all our breaking changes out now so once 1.0 is solid, we don't break stuff. We can open a new issue for this

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.

validate_numeric condition confusion
3 participants