Closed richmolj closed 6 years ago
@wadetandy
Looks good. Do you want to downcase everything before comparison?
Probably not. Not against it if we get an ask for it, but I mean, if you’re upset because the string TRUE didnt become the boolean true...im not sure what to say. Even though it’s a minor code change, I tend to lean against a feature until it becomes a requirement (YAGNI and so forth)
But good comment for sure.
Yeah good enough for me just making sure it was a conscious choice.
One more thing I just thought of: should this normalization logic exist at the filter layer or at the adapter layer? I could see this being a much more backend-specific transformation in certain cases, with this one being a good default for the active record adapter. Moving to that layer would also allow for additional enhancements like conditional normalization based on whether the model attribute was a Boolean or string type. Not particularly attached to either option.
I think there might be adapter-specific logic, but that wouldn't be mutually exclusive to this commit. I think all these would be basic expectations of users. Almost more a bug fix than anything else IMO, whereas the adapter stuff would be more like additional features.
"true"
becomestrue
"false"
becomesfalse
"nil"
becomesnil
"null"
becomesnil