Aspect Ratio Validator error message should present aspect ratios as W:H, not WxH. #161
Conversation
The nested `else` at the end of the aspect ratio validator decision tree is unnecessary because `when` can take a regex directly. Additionally, the `_` character doesn't need to be escaped in regexes.
|
Tagging @corrieleech so she gets notified when this gets merged. |
| if options[:with] =~ /is\_(\d*)\_(\d*)/ | ||
| x = $1.to_i | ||
| y = $2.to_i | ||
| when /is_(\d*)_(\d*)/ |
There was a problem hiding this comment.
These character groups should probably be (\d+) as is__1 or is_5_ would be currently allowed.
There was a problem hiding this comment.
when /\Ais_(\d*[1-9]\d*)_(\d*[1-9]\d*)\z/would also ensure the string is exactly is_X_Y with X and Y being > 0.
There was a problem hiding this comment.
I used /\Ais_([1-9]\d*)_([1-9]\d*)\z/ which is, I think, a bit better since it will not accept ratios like is_01_2
|
|
||
| return true if (x.to_f / y).round(PRECISION) == (metadata[:width].to_f / metadata[:height]).round(PRECISION) | ||
| return true if (x.to_f / y).round(PRECISION) == (metadata[:width].to_f / metadata[:height]).round(PRECISION) |
There was a problem hiding this comment.
If y will be supplied as 0 this line will cause a devision by zero. What do you think about adding that as condition?
return true if !y.zero? && (x.to_f / y).round(PRECISION) == (metadata[:width].to_f / metadata[:height]).round(PRECISION)The code will then default into the invalid aspect ratio error.
There was a problem hiding this comment.
Done :) even if this case should not occur with the new regex
|
@yarmiganosca I commented on two things, what do you think about them? If you could fix the conflicts as well I'd totally second the PR. 👍 |
|
@yarmiganosca PR looks good, and I also think that suggestions from @gr8bit make sense (since I know you just moved my personal code a little :)) if you can quickly apply suggestions I'll merge your PR |
|
oh, and also - you need to resolve conflicts :( |
|
@yarmiganosca are you still available for these fixes? Please let me know, I could finish this PR if you aren't. :) |
|
Sorry, I had completely forgotten I submitted this PR, so I thought I was just receiving messages from some repo I had commented on, not actually submitted the entire, ya know, PR. My apologies! I'll take a look later today. |
Sorry it took us so long! I'm glad we'll be able to merge these change soon. :) |
|
@gr8bit I think for misconfigurations of the validator itself (and I think what you're referring to would be a misconfiguration of the validator itself), we should try to detect those when the validator is declared on the model, not when validating individual records. I see we already have a check_validity! method, and it's my understanding that Rails will call that itself when a validation is declared on a model (at least I hope that's the case, because afaict check_validity! isn't actually called anywhere in this gem). If that understand is correct, then all these concerns you have are quite valid, but I think they should be addressed there instead of when we're validating individual records. What do you think? |
|
@yarmiganosca A very good point! I absolutely favor the approach of validating the input on declaration instead of during runtime. |
|
@yarmiganosca are you available to add the suggested validations to the validator configuration? I'd then gladly merge it. =) |
|
@yarmiganosca can you make the changes that @gr8bit required so we can merge this PR ? :) |
|
@igorkasyanchuk @gr8bit FYI, I took the initiative to complete this PR since we have no news from @yarmiganosca :/ |
|
@yarmiganosca & @corrieleech FYI this will be released in the coming days / weeks |
Fixes #160
I also did a very small amount of refactoring, by removing and unnecessary double-nested
elseand unnecessary escape characters from the aspect ratio regex.