Rubocop fixes for MiqExpression #15488

imtayadeway · 2017-06-30T18:09:25Z

Fixes all but one complaints (over the eval, which it should probably continue to complain about 😉 )

Probably best viewed as individual commits

@miq-bot add-label core, technical debt
@miq-bot assign @gtanzillo

Fryguy · 2017-06-30T19:51:09Z

lib/miq_expression.rb

  BASE_TABLES = config[:base_tables]
  INCLUDE_TABLES = config[:include_tables]
  EXCLUDE_COLUMNS = config[:exclude_columns]
  EXCLUDE_EXCEPTIONS = config[:exclude_exceptions]
  TAG_CLASSES = config[:tag_classes]
  EXCLUDE_FROM_RELATS = config[:exclude_from_relats]
  FORMAT_SUB_TYPES = config[:format_sub_types]
-  FORMAT_BYTE_SUFFIXES = FORMAT_SUB_TYPES[:bytes][:units].inject({}) { |h, (v, k)| h[k] = v; h }
+  FORMAT_BYTE_SUFFIXES = FORMAT_SUB_TYPES[:bytes][:units].each_with_object({}) { |(v, k), h| h[k] = v }


In this particular case, this feels cleaner to just do

FORMAT_BYTE_SUFFIXES = FORMAT_SUB_TYPES[:bytes][:units].to_h.invert

or if you don't like the invert for whatever reason

FORMAT_BYTE_SUFFIXES = FORMAT_SUB_TYPES[:bytes][:units].transpose.reverse.transpose.to_h

;)

Nice! Didn't know about invert

invert is nice if you are sure that your values are also unique (which in this case, we know they are because the original code was essentially doing the same thing)

Fryguy · 2017-06-30T19:55:14Z

lib/miq_expression.rb

@@ -96,13 +97,13 @@ def self._to_human(exp, options = {})
    when "between dates", "between times"
      col_name = exp[operator]["field"]
      col_type = get_col_type(col_name)
-      col_human, dumy = operands2humanvalue(exp[operator], options)
+      col_human, = operands2humanvalue(exp[operator], options)


I know it's proper Ruby, but whenever people come across this and don't know Ruby can do this, they get confused. I think it's more readable (and still rubocop compliant) to do:

col_human, _ = operands2humanvalue(exp[operator], options)

or

col_human, _val_human = operands2humanvalue(exp[operator], options)

(or whatever the right-hand-value is actually called)

WDYT about global disabling the rule that complains about your preferred version?

which rule is it (link?)

Fryguy · 2017-06-30T19:56:24Z

lib/miq_expression.rb

@@ -392,7 +392,7 @@ def self.get_cols_from_expression(exp, options = {})
      elsif exp.key?("count")
        result[exp["count"]] = get_col_info(exp["count"], options)
      elsif exp.key?("tag")
-        # ignore
+      # ignore


The original was actually better. Rubocop complains but might not if you put a nil here..

elsif exp.key?("tag") nil # ignore else

either that or collapse this with the else

elsif !exp.key?("tag") exp.each_value { |v| result.merge!(get_cols_from_expression(v, options)) } end

Oops. This was a mistake! I'll put this back as it wasn't a cop complaint

Fryguy · 2017-06-30T19:58:22Z

lib/miq_expression.rb

        classification = options[:classification] || Classification.find_by_name(col)
+        # rubocop:enable Rails/DynamicFindBy


You can inline this without the explicit disable/enable pair

classification = options[:classification] || Classification.find_by_name(col) # rubocop:disable Rails/DynamicFindBy

Fryguy · 2017-06-30T19:58:42Z

lib/miq_expression.rb

  config = YAML.load(ERB.new(File.read(Rails.root.join("config", "miq_expression.yml"))).result)
+  # rubocop:enable Security/YAMLLoad


You can inline this without the explicit disable/enable pair

Fryguy · 2017-06-30T19:59:22Z

lib/miq_expression.rb

@@ -666,7 +672,8 @@ def self.quote_human(val, typ)
    when "integer", "decimal", "fixnum", "float"
      return val.to_i unless val.to_s.number_with_method? || typ.to_s == "float"
      if val =~ /^([0-9\.,]+)\.([a-z]+)$/
-        val = $1; sfx = $2
+        val = $1
+        sfx = $2


Alternately,

val, sfx = $1, $2

Fryguy · 2017-06-30T20:00:00Z

lib/miq_expression.rb

@@ -678,7 +685,7 @@ def self.quote_human(val, typ)
    when "string", "date", "datetime"
      return "\"#{val}\""
    else
-      return quote(val, typ)
+      quote(val, typ)


If you are killing the return here, you can kill it in every other conditional above

Yeah I thought this too....I kind of had blinders on only addressing the ones the cop found, but I'll nix these too

yeah, I figured as much

Fryguy · 2017-06-30T20:01:18Z

lib/miq_expression.rb

-        if c.match(excl)
-          col = nil
-          break
+      unless EXCLUDE_EXCEPTIONS.include?(c)


I'm not sure why this is needed when it's checked a few lines up

Oh weird I must have screwed this up when rebasing. Fix coming up!

the original code had it as well, so I don't think you screwed it up, I was just looking at it without those blinders on 🕶

Actually I think this is equivalent to before, I was seeing something else ;) I'm finding this way too confusing to think about, perhaps I can look into this in a follow up?

Fryguy · 2017-06-30T20:02:18Z

lib/miq_expression.rb

+                 field
+               else
+                 get_col_type(field.to_s) || :string
+               end


My personal preference, but I can't stand the super indents...I much prefer

col_type = if field == :count || field == :regkey field else get_col_type(field.to_s) || :string end

There are a few like that here...what do you think?

I'm OK with that 👍 (more specifically, Matz' auto indent rules are OK with that ;))

Fryguy · 2017-06-30T20:02:53Z

lib/miq_expression.rb

      catobj = Classification.find_by_name(cat)
+      # rubocop:enable Rails/DynamicFindBy


inline the disable

Fryguy · 2017-06-30T20:08:57Z

lib/miq_expression.rb

@@ -1229,8 +1252,6 @@ def fields(expression = exp)
    end
  end

-  private
-


Although it didn't actually make the ruby_for_date_compare method private, I believe the intention is for the method to be private. Thus, I think that you need to add a private_class_method :ruby_for_date_compare

Fryguy · 2017-06-30T20:09:38Z

lib/miq_expression.rb

-      last_path = ref.collection? ? association_class.model_name.plural : association_class.model_name.singular
-    end
-    last_path
-  end


Why is this being make public? (Yes, I know it was not actually private, but the intention seems for it to have been a private_class_method)

I think there are some tests for this that invoke it directly....would you still prefer to declare it private and use send in the tests?

yes, if it's only tests, then I prefer keeping it private, make the test call send, and somewhere in the describe line note that it's testing a private method...I usually do something like

describe ".determine_relat_path (private)" do

Fryguy · 2017-06-30T20:10:03Z

Cool stuff, @imtayadeway !

imtayadeway · 2017-06-30T20:40:59Z

Thanks for the close 👀 , @Fryguy ! Amendments coming up....

This stuff is controlled by us so it isn't a security risk.

It's our method, not legacy Rails

Fryguy · 2017-06-30T22:01:51Z

Style/TrailingUnderscoreVariable 🙄

Do we know what the second return value is supposed to be? I've always preferred code that was explicit in this case by prefixing with _

imtayadeway · 2017-06-30T22:03:08Z

@Fryguy yeah I can update to have a descriptive _variable 👍

miq-bot · 2017-07-06T16:38:08Z

Checked commits imtayadeway/manageiq@5af676d~...072ebcb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

lib/miq_expression.rb

❗ - Line 650, Col 17 - Security/Eval - The use of eval is a serious security risk.

gtanzillo

LGTM 👍

miq-bot added core technical debt labels Jun 30, 2017

miq-bot assigned gtanzillo Jun 30, 2017

Fryguy reviewed Jun 30, 2017

View reviewed changes

lib/miq_expression.rb Outdated

catobj = Classification.find_by_name(cat)

# rubocop:enable Rails/DynamicFindBy

Copy link

Member

Fryguy Jun 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline the disable

Fryguy reviewed Jun 30, 2017

View reviewed changes

imtayadeway added 10 commits June 30, 2017 14:29

Disable Security/YAMLLoad cop

5af676d

This stuff is controlled by us so it isn't a security risk.

Prefer to_h.invert over inject

1becbeb

Avoid terminating with ;

168d302

Avoid useless assignment

a7742d2

Prefer assigning to return of conditional

6ac7220

Avoid { and } for multiline blocks

64dbed5

Prefer empty? over size == 0

f83deae

Put a space between do and |

5ed1b58

Disable DynamicFindBy cop

b32c465

It's our method, not legacy Rails

Remove unnecessary parentheses

fd786dc

imtayadeway force-pushed the rubocop/fix-miq-expression branch 2 times, most recently from d3cabf4 to f2e7b0f Compare June 30, 2017 21:52

Fryguy approved these changes Jun 30, 2017

View reviewed changes

imtayadeway added 15 commits July 6, 2017 09:36

Use private_class_method to make these private

db21b42

Remove rendundant self

2eeac54

Prefer \ for multiline strings

e7ed6e3

Avoid unnecessary return

c56e167

Fix inconsistent whitespace

08d3439

Remove useless assignment

6f62414

Prefer next to skip iteration

d320d2f

s/get_categories/categories/

ee10bbc

Prefer begin/end blocks for rescuing

423f484

Prefer numeric?/integer? to is_numeric?/is_integer?

9e7bf32

Prefer %w for array of words

4bbfecb

Prefer class instance variables

d172f49

Avoid unless modifier in multiline statement

4fb5bc7

Prefer elsif to else/if

d06e557

Prefer named unused variable in parallel assignment

072ebcb

imtayadeway force-pushed the rubocop/fix-miq-expression branch from f2e7b0f to 072ebcb Compare July 6, 2017 16:36

gtanzillo approved these changes Jul 12, 2017

View reviewed changes

gtanzillo added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 12, 2017

gtanzillo merged commit 7659538 into ManageIQ:master Jul 12, 2017

kbrock mentioned this pull request Apr 17, 2018

[FINE] convert Vm#miq_provision_template to has_one #17309

Merged

		classification = options[:classification] \|\| Classification.find_by_name(col)
		# rubocop:enable Rails/DynamicFindBy

		config = YAML.load(ERB.new(File.read(Rails.root.join("config", "miq_expression.yml"))).result)
		# rubocop:enable Security/YAMLLoad

		catobj = Classification.find_by_name(cat)
		# rubocop:enable Rails/DynamicFindBy

@@ @@ -1229,8 +1252,6 @@ def fields(expression = exp) @@
                   end
                 end
-                private

Rubocop fixes for MiqExpression #15488

Rubocop fixes for MiqExpression #15488

Conversation

imtayadeway commented Jun 30, 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

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

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

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

Choose a reason for hiding this comment

Fryguy Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Fryguy commented Jun 30, 2017

imtayadeway commented Jun 30, 2017

Fryguy commented Jun 30, 2017

imtayadeway commented Jun 30, 2017

miq-bot commented Jul 6, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

Fryguy Jun 30, 2017 •

edited

Loading