Numerics shouldn't be tested for ".bytes" like suffix #23349

jrafanie · 2025-02-20T20:16:11Z

Symbols or other non-strings should be converted to strings before testing the regex.

lib/miq_expression.rb

Fryguy · 2025-02-21T13:57:18Z

lib/miq_expression.rb

@@ -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


I feel like this line should have handled your specific case, so I'm not sure why it didn't.

unless or.. let me check it out... need a truth table

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 end

which should be the same as the added guard clause

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.

jrafanie · 2025-02-21T21:00:10Z

lib/miq_expression.rb

-        else
-          "#{val} #{sfx.titleize}"
-        end
+      /^([0-9.,]+)\.([a-z]+)$/.match(val.to_s)


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]+)$/

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.

[1] pry(main)> "a" =~ /[.]/ => nil [2] pry(main)> "." =~ /[.]/ => 0 [3] pry(main)> "a" =~ /[\.]/ => nil [4] pry(main)> "." =~ /[\.]/ => 0

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

jrafanie · 2025-02-25T22:58:41Z

Pushed with String::NUMBER_WITH_METHOD_REGEX changes

Fryguy · 2025-02-26T15:24:06Z

spec/lib/miq_expression_spec.rb

+      expect(exp.to_human).to eq("VM and Instance.Hardware.Volumes : Free Space Percent > 75")
+    end
+
+    it "generates a human readable string for a numeric FIELD value expression" do


This text is the same as the previous test, so I think change to

Suggested change

it "generates a human readable string for a numeric FIELD value expression" do

it "generates a human readable string for a float FIELD value expression" do

Fryguy · 2025-02-26T15:24:46Z

lib/miq_expression.rb

+      String::NUMBER_WITH_METHOD_REGEX.match(val.to_s)
+      val, sfx = $1, $2


If we're using .match, then we shouldn't use $ variables. Prefer

Suggested change

String::NUMBER_WITH_METHOD_REGEX.match(val.to_s)

val, sfx = $1, $2

val, sfx = String::NUMBER_WITH_METHOD_REGEX.match(val.to_s).captures

Fryguy · 2025-02-26T15:25:56Z

A couple more smaller comments, and this is good to go. The tests are great.

Reuse existing NUMBER_WITH_METHOD_REGEX.match from more core extensions instead of using a very similar regex.

jrafanie · 2025-02-26T17:22:35Z

spec/lib/miq_expression_spec.rb

+      expect(exp.to_human).to eq("VM and Instance.Hardware.Volumes : Free Space Percent > 75")
+    end
+
+    it "generates a human readable string for a float FIELD value expression" do


in b4 "sort these tests"... it's sorted by expected formats (basic numeric first, then, float, then, symbol "to i with method" format).

🤣

jrafanie added the bug label Feb 20, 2025

jrafanie requested a review from Fryguy as a code owner February 20, 2025 20:16

jrafanie force-pushed the fix-expression-to-human-with-numeric-value branch from 07ca05e to 1e25ed3 Compare February 20, 2025 20:23

Fryguy reviewed Feb 20, 2025

View reviewed changes

lib/miq_expression.rb Outdated Show resolved Hide resolved

Fryguy approved these changes Feb 20, 2025

View reviewed changes

Fryguy reviewed Feb 20, 2025

View reviewed changes

lib/miq_expression.rb Show resolved Hide resolved

jrafanie force-pushed the fix-expression-to-human-with-numeric-value branch 2 times, most recently from 5fe17ef to 11a32fb Compare February 20, 2025 22:11

Fryguy reviewed Feb 21, 2025

View reviewed changes

Fryguy self-assigned this Feb 21, 2025

jrafanie commented Feb 21, 2025

View reviewed changes

Numerics shouldn't be tested for ".bytes" like suffix

4c159c9

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 d4bdbe2 to 0af1900 Compare February 25, 2025 22:55

Fryguy reviewed Feb 26, 2025

View reviewed changes

number_with_method? should handle floats to simplify this

7bcf88c

Reuse existing NUMBER_WITH_METHOD_REGEX.match from more core extensions instead of using a very similar regex.

jrafanie force-pushed the fix-expression-to-human-with-numeric-value branch from 0af1900 to 7bcf88c Compare February 26, 2025 17:16

jrafanie commented Feb 26, 2025

View reviewed changes

Fryguy approved these changes Feb 26, 2025

View reviewed changes

Fryguy merged commit c7cf128 into ManageIQ:master Feb 26, 2025
8 checks passed

jrafanie deleted the fix-expression-to-human-with-numeric-value branch February 28, 2025 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Numerics shouldn't be tested for ".bytes" like suffix #23349

Numerics shouldn't be tested for ".bytes" like suffix #23349

jrafanie commented Feb 20, 2025

Fryguy Feb 21, 2025

jrafanie Feb 21, 2025

jrafanie Feb 21, 2025

Fryguy Feb 21, 2025 •

edited

Loading

jrafanie Feb 21, 2025

Fryguy Feb 21, 2025

Fryguy Feb 21, 2025 •

edited

Loading

Fryguy Feb 21, 2025 •

edited

Loading

jrafanie commented Feb 25, 2025

Fryguy Feb 26, 2025

Fryguy Feb 26, 2025

Fryguy commented Feb 26, 2025

jrafanie Feb 26, 2025

	it "generates a human readable string for a numeric FIELD value expression" do
	it "generates a human readable string for a float FIELD value expression" do

		String::NUMBER_WITH_METHOD_REGEX.match(val.to_s)
		val, sfx = $1, $2

	String::NUMBER_WITH_METHOD_REGEX.match(val.to_s)
	val, sfx = $1, $2
	val, sfx = String::NUMBER_WITH_METHOD_REGEX.match(val.to_s).captures

Numerics shouldn't be tested for ".bytes" like suffix #23349

Numerics shouldn't be tested for ".bytes" like suffix #23349

Conversation

jrafanie commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Fryguy Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

jrafanie commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Feb 26, 2025

Choose a reason for hiding this comment

Fryguy Feb 21, 2025 •

edited

Loading

Fryguy Feb 21, 2025 •

edited

Loading

Fryguy Feb 21, 2025 •

edited

Loading