Numerics shouldn't be tested for ".bytes" like suffix #23349
Conversation
jrafanie
force-pushed
the
fix-expression-to-human-with-numeric-value
branch
from
07ca05e to
1e25ed3
Compare
jrafanie
force-pushed
the
fix-expression-to-human-with-numeric-value
branch
from
1e25ed3 to
5fe17ef
Compare
Symbols or other non-strings should be converted to strings before testing the regex. Fixes ManageIQ/manageiq-ui-classic#9357
jrafanie
force-pushed
the
fix-expression-to-human-with-numeric-value
branch
from
5fe17ef to
11a32fb
Compare
lib/miq_expression.rb
Outdated
| @@ -804,9 +804,10 @@ def self.quote(val, typ) | |||
| def self.quote_human(val, typ) | |||
| case typ&.to_sym | |||
| when :integer, :decimal, :fixnum, :float | |||
| return val if val.kind_of?(Numeric) | |||
| return val.to_i unless val.to_s.number_with_method? || typ == :float | |||
There was a problem hiding this comment.
I feel like this line should have handled your specific case, so I'm not sure why it didn't.
There was a problem hiding this comment.
unless or.. let me check it out... need a truth table
There was a problem hiding this comment.
So, both examples are for columns that are floats columns so typ is float. I think I can just remove the || typ == :float and my first guard clause should handle it.
I believe all floats end up in on line 817/818:
else
val
endwhich should be the same as the added guard clause
There was a problem hiding this comment.
Maybe this line is just for strings, but not strings that are like 5.bytes e.g. "5". The || typ == :float hurts my brain, though.
Even so, then I'd expect the cases of "5" and 5 to be the same, returning val.to_i, and so that wouldn't fall through into the next line, so I'm confused.
| else | ||
| "#{val} #{sfx.titleize}" | ||
| end | ||
| /^([0-9.,]+)\.([a-z]+)$/.match(val.to_s) |
There was a problem hiding this comment.
Review this with whitespace removed. I think it simplifies it. I was surprised I could not use $1, $2 after line 808 (number_with_method?) since that returns the result of =~ anyway but alas, they were not set, perhaps when the scope returns out of that method. It feels redundant to use a nearly identical regex.
btw, any idea why they're different?
https://github.com/ManageIQ/more_core_extensions/blob/27d3e2086d26d7814a1fdeaf9ba7a7281344fab8/lib/more_core_extensions/core_ext/string/to_i_with_method.rb#L3C5-L3C64
number_with_method?
/^([0-9\.,]+)\.([a-z]+)$/vs.
this code:
/^([0-9.,]+)\.([a-z]+)$/There was a problem hiding this comment.
Those 2 are identical. a . within [] is treated literally. Technically the former doesn't need to escape it, but I find it more explicit when it does, and is clearer to the reader, IMO.
There was a problem hiding this comment.
[1] pry(main)> "a" =~ /[.]/
=> nil
[2] pry(main)> "." =~ /[.]/
=> 0
[3] pry(main)> "a" =~ /[\.]/
=> nil
[4] pry(main)> "." =~ /[\.]/
=> 0There was a problem hiding this comment.
I was surprised I could not use $1, $2 after line 808 (number_with_method?) since that returns the result of =~ anyway
Yeah the regex side effects are local scope, if I recall. Even so, number_with_method? should return boolean, and we should provide a separate method that returns the constituent parts (perhaps named number_with_method (no ?).
EDIT, or we can just use NUMBER_WITH_METHOD_REGEX directly.
Symbols or other non-strings should be converted to strings before testing the regex.
Fixes ManageIQ/manageiq-ui-classic#9357