add decimal_si_to_f #43

zeari · 2017-06-05T11:29:10Z

Needed in order to handle https://github.com/kubernetes/kubernetes/blob/4def5add114b651c26fe576b7315f8029bfce46a/vendor/github.com/appc/spec/schema/types/resource/quantity.go#L49
@cben @moolitayer @zakiva Please review
cc @simon3z

cben · 2017-06-05T11:32:40Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

@@ -0,0 +1,19 @@
+module MoreCoreExtensions
+  module DECIMAL_SI


Why capital? Following IEC60027_2 example? I think that's because IEC is an abbreviation, so this would be DecimalSI.
But module & filename should match, so module DecimalSuffix?

👍 on both points

Or, name the file decimal_si.rb

cben · 2017-06-05T11:40:16Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

+module MoreCoreExtensions
+  module DECIMAL_SI
+    # made to handle https://github.com/kubernetes/kubernetes/blob/4def5add114b651c26fe576b7315f8029bfce46a/vendor/github.com/appc/spec/schema/types/resource/quantity.go#L49
+    DECIMAL_SUFFIXES = %w(k M G T P E).freeze


a hash {"m" => 1e-3, "k" => 1e3, "M" => 1e6, ...} might make the logic simpler.

👍 for this...I prefer the Hash notation.

Additionally, if you use the Hash, you can use a Hash default of 1 to handle when there is no suffix (the elsif suffix_index.nil? condition below).

cben · 2017-06-05T11:40:36Z

spec/core_ext/string/decimal_suffix_spec.rb

@@ -0,0 +1,12 @@
+describe String do
+  it '#iec60027_2' do


it 'decimal_si_to_f'

simon3z · 2017-06-05T12:12:23Z

@cben @zeari remember that Quantity is a fixed point variable, it would be nice to try and keep it fixed (vs current floating).

moolitayer · 2017-06-05T14:33:51Z

@cben @zeari remember that Quantity is a fixed point variable, it would be nice to try and keep it fixed (vs current floating).

In that case we should also check the column type this is eventually stored into

simon3z · 2017-06-06T17:08:03Z

In that case we should also check the column type this is eventually stored into

@moolitayer what do you mean by "check the column type"? when you do a to_f or a to_i how do you check the column type that you'll use for those?

moolitayer · 2017-06-07T09:16:56Z

@moolitayer what do you mean by "check the column type"? when you do a to_f or a to_i how do you check the column type that you'll use for those?

The numeric types of the columns these are stored into in Postrgres need not be real or double precision AFAIK

simon3z · 2017-06-07T10:06:34Z

The numeric types of the columns these are stored into in Postrgres need not be real or double precision AFAIK

@moolitayer yes, that's correct but it's out of scope here (generic lib).

zeari · 2017-06-08T13:06:22Z

@moolitayer @simon3z IIUC that means i would have to convert it to the smallest possible unit which is "m" = mili. is that what we want here? because that would be weird for bytes

zeari · 2017-06-08T14:38:48Z

we can also go with something like string.decimal_si_to_f(desired_unit) and desired unit would be 1 of "m", "k", "M".... "none" and itll convert it to that unit

Fryguy · 2017-06-13T15:56:00Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

@@ -0,0 +1,19 @@
+module MoreCoreExtensions
+  module DECIMAL_SI
+    # made to handle https://github.com/kubernetes/kubernetes/blob/4def5add114b651c26fe576b7315f8029bfce46a/vendor/github.com/appc/spec/schema/types/resource/quantity.go#L49


I don't think we need a kubernetes reference comment for something that is supposed to be generic. This comment might be appropriate in the body of the commit message, though.

Additionally, the reference is pointing to a vendored package within kubernetes, so it might be better to link the original package instead.

Fryguy · 2017-06-13T16:08:36Z

we can also go with something like string.decimal_si_to_f(desired_unit) and desired unit would be 1 of "m", "k", "M".... "none" and itll convert it to that unit

ActiveSupport doesn't have these reverse converters, instead choosing to implement them at the presentation layer via the NumberHelper view helper. We implemented more of these inManageIQ's NumberHelper as well.

I'm not sure what is the "right" approach for these reverse converters.

EDIT: It seems ActiveSupport patches to_s in order to expose the NumberHelper methods. https://api.rubyonrails.org/classes/ActiveSupport/NumericWithFormat.html This is really interesting and probably something we might want to extend for ManageIQ's additional number helpers

simon3z · 2017-06-17T23:21:48Z

I saw good comments about style and generalization (that should be addressed).
Anyway ATM overall 👍 for the case we need to cover.
So feel free to merge when ready.

simon3z · 2017-06-17T23:26:56Z

@moolitayer @simon3z IIUC that means i would have to convert it to the smallest possible unit which is "m" = mili. is that what we want here? because that would be weird for bytes

@zeari yes for our use-case I would have preferred to go that way (use milli as unit). But that seems very specific to our use-case.

Anyway either we use something very specific (in the kubernetes parser) or we go with something more generic as this. I understand the pros/cons.

I don't think we should hold this PR on the above discussion (that maybe we can continue somewhere else).

zeari · 2017-06-18T14:23:47Z

Comments addressed. This also handles decimal exponents.

tests included

zeari · 2017-06-19T14:11:37Z

ManageIQ/manageiq-providers-kubernetes#22 is also coordinated with this

moolitayer

LGTM 👍

moolitayer · 2017-06-19T14:58:53Z

BTW method can be written as

float(to_i) *
{
  "m" => 1e-3, "k" => 1e3, "M" => 1e6, "G" => 1e9, "T" => 1e12, "P" => 1e15, "E" => 1e18, nil => 1
}[
  self[-1]
]

but that might be more confusing...

bdunne

With this extension and iec60027_2, it feels like it would be nicer to have one method that handled all of the cases. Maybe something like string_to_numeric. What do you think @zeari ?

bdunne · 2017-06-28T18:41:47Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

@@ -0,0 +1,15 @@
+module MoreCoreExtensions
+  module DecimalSI
+    DECIMAL_SUFFIXES = {"m" => 1e-3, "k" => 1e3, "M" => 1e6, "G" => 1e9, "T" => 1e12, "P" => 1e15, "E" => 1e18}.freeze


Can you add all of the suffixes?
https://en.wikipedia.org/wiki/Metric_prefix

bdunne · 2017-06-28T18:48:31Z

spec/core_ext/string/decimal_suffix_spec.rb

+    expect("1T".decimal_si_to_f).to eq(1_000_000_000_000.0)
+    expect("1P".decimal_si_to_f).to eq(1_000_000_000_000_000.0)
+    expect("1E".decimal_si_to_f).to eq(1_000_000_000_000_000_000.0)
+    expect("1e9".decimal_si_to_f).to eq(1_000_000_000.0)


Is this test added to verify that it's not broken?

yep. Since I depend on this method to do both.

bdunne · 2017-06-28T18:48:43Z

spec/core_ext/string/decimal_suffix_spec.rb

+    expect("1P".decimal_si_to_f).to eq(1_000_000_000_000_000.0)
+    expect("1E".decimal_si_to_f).to eq(1_000_000_000_000_000_000.0)
+    expect("1e9".decimal_si_to_f).to eq(1_000_000_000.0)
+    expect("1e-9".decimal_si_to_f).to eq(0.000_000_001)


bdunne · 2017-06-28T19:17:23Z

lib/more_core_extensions/core_ext/string/decimal_suffix.rb

+    DECIMAL_SUFFIXES = {"m" => 1e-3, "k" => 1e3, "M" => 1e6, "G" => 1e9, "T" => 1e12, "P" => 1e15, "E" => 1e18}.freeze
+    def decimal_si_to_f
+      suffix = self[-1]
+      if DECIMAL_SUFFIXES[suffix].nil?


It reads nicer in the positive case.

multiplier = DECIMAL_SUFFIXES[suffix] if multiplier Float(self[0..-2]) * multiplier else Float(self) end

zeari · 2017-06-29T08:44:38Z

With this extension and iec60027_2, it feels like it would be nicer to have one method that handled all of the cases. Maybe something like string_to_numeric. What do you think @zeari ?

Sure, lets get this in and do that in separate PR though.

miq-bot · 2017-06-29T10:20:21Z

Checked commit zeari@fd28b8b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

zeari · 2017-06-29T13:44:55Z

@bdunne all comments addressed

zeari · 2017-07-02T10:41:50Z

ping @bdunne @Fryguy

Fryguy

LGTM @bdunne Please merge if you are good.

zeari · 2017-07-17T16:04:07Z

@bdunne When can this be available on master?

Fryguy · 2017-07-18T17:55:02Z

@zeari We are just getting in a few more extractions from ManageIQ, and then we plan to cut a release. I'm hoping in the next couple of days.

bdunne · 2017-07-24T14:10:53Z

@zeari Version 3.3.0 has been released with these changes

zeari · 2017-07-24T14:26:42Z

Thanks @bdunne @Fryguy

zeari changed the title ~~Decimal si~~ add decimal_si_to_f Jun 5, 2017

cben reviewed Jun 5, 2017

View reviewed changes

chessbyte assigned Fryguy Jun 5, 2017

Fryguy reviewed Jun 13, 2017

View reviewed changes

zeari force-pushed the decimal_si branch 4 times, most recently from dcd674a to 73e4a78 Compare June 18, 2017 10:51

zeari mentioned this pull request Jun 19, 2017

Collect limit items for container definitions ManageIQ/manageiq#15180

Closed

moolitayer approved these changes Jun 19, 2017

View reviewed changes

bdunne requested changes Jun 28, 2017

View reviewed changes

zeari force-pushed the decimal_si branch from 73e4a78 to 64287a0 Compare June 29, 2017 09:04

add decimal_si_to_f: convert decimal suffixes (k, M, G...) to float

fd28b8b

zeari force-pushed the decimal_si branch from 64287a0 to fd28b8b Compare June 29, 2017 10:15

Fryguy approved these changes Jul 3, 2017

View reviewed changes

bdunne approved these changes Jul 3, 2017

View reviewed changes

bdunne merged commit b4bd8d3 into ManageIQ:master Jul 3, 2017

bdunne added the enhancement label Jul 3, 2017

zeari mentioned this pull request Jul 18, 2017

collect container definition limits ManageIQ/manageiq-providers-kubernetes#22

Merged

add decimal_si_to_f #43

add decimal_si_to_f #43

Conversation

zeari commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Jun 5, 2017

moolitayer commented Jun 5, 2017

simon3z commented Jun 6, 2017

moolitayer commented Jun 7, 2017

simon3z commented Jun 7, 2017

zeari commented Jun 8, 2017 • edited Loading

zeari commented Jun 8, 2017 • edited Loading

Fryguy Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jun 13, 2017 • edited Loading

simon3z commented Jun 17, 2017

simon3z commented Jun 17, 2017

zeari commented Jun 18, 2017 • edited Loading

zeari commented Jun 19, 2017

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer commented Jun 19, 2017

bdunne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Jun 29, 2017

miq-bot commented Jun 29, 2017

zeari commented Jun 29, 2017

zeari commented Jul 2, 2017

Fryguy left a comment

Choose a reason for hiding this comment

zeari commented Jul 17, 2017

Fryguy commented Jul 18, 2017

bdunne commented Jul 24, 2017

zeari commented Jul 24, 2017 • edited Loading

zeari commented Jun 8, 2017 •

edited

Loading

zeari commented Jun 8, 2017 •

edited

Loading

Fryguy Jun 13, 2017 •

edited

Loading

Fryguy commented Jun 13, 2017 •

edited

Loading

zeari commented Jun 18, 2017 •

edited

Loading

zeari commented Jul 24, 2017 •

edited

Loading