active_storage_validations
active_storage_validations copied to clipboard
igorkasyanchuk
Aspect Ratio Validator error message should present aspect ratios as W:H, not WxH.
Fixes #160
I also did a very small amount of refactoring, by removing and unnecessary double-nested else and unnecessary escape characters from the aspect ratio 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
@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, 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 :/ I also added the validity check on the passed aspect_ratio as mentioned in @yarmiganosca last comment
@yarmiganosca & @corrieleech FYI this will be released in the coming days / weeks