Un Seattle #15561

bdunne · 2017-07-12T22:25:49Z

While building ManageIQ/guides#228 I tested against this repo and here are the changes. rubocop -a did almost everything here. There were only a few that I had to manually fix because autocorrect would blow up on them.

I also ignored the following in this PR, but didn't add exceptions for them since I believe they are deprecated, but didn't want to introduce a lot of code change here.

validates_format_of
validates_inclusion_of
validates_presence_of
validates_uniqueness_of

Thanks @NickLaMuro for inspiring me to search for this cop again 😄

NickLaMuro · 2017-07-12T22:30:09Z

I have created a monster... 😲

NickLaMuro

From a technical standpoint, I think 99% of this is fine, and I only disagree with a few of these changes syntactically speaking (most are probably subjective).

But I am only going to "Comment" on this PR for a review, because objectively speaking I disagree with this PR and it's sister PR ManageIQ/guides#228, but that is from a personal standpoint. I get "why" you are pushing for this from a project maintenance standpoint, but that doesn't mean I have to like it... (honestly... makes me feel like I am writing Javascript or Go...)

That said, I would say that half of these are log.X statements, with some throw, yield and others tossed in there as well, and those updates probably are overkill for what you are trying to accomplish. But that is probably up to the opinions of those reviewing ManageIQ/guides#228, so I will leave the decisions regarding those bits to the reviewers of 228. Brought up what I thought was relevant to that PR in this review, but will leave it up to you and the others to determine what you chose to act on from that.

NickLaMuro · 2017-07-12T23:52:26Z

tools/rest_api.rb

-        version "#{API_CMD} #{VERSION} - ManageIQ REST API Access Script"
-        banner <<-EOS
+        version("#{API_CMD} #{VERSION} - ManageIQ REST API Access Script")
+        banner(<<-EOS)


I think putting HEREDOC statements in parens looks awful and is quiet confusing. It also doesn't conform from what I assumes one of it's roots is, bash/shell programming: http://tldp.org/LDP/abs/html/here-docs.html

NickLaMuro · 2017-07-12T23:52:46Z

tools/column_ordering/column_ordering.rb

@@ -98,7 +98,7 @@ def table_dump

    pg_dump_result = AwesomeSpawn.run("pg_dump", :params => params)

-    raise ColumnOrderingError <<-ERROR.gsub!(/^ +/, "") if pg_dump_result.failure?
+    raise ColumnOrderingError(<<-ERROR.gsub!(/^ +/, "")) if pg_dump_result.failure?


In the case of these, I would say these errors should be made into local variables, and loaded into this method, instead of being defined inside of the raise call:

if pg_dump_result.failure? err_message = <<-ERROR.gsub!(/^ +/, "") '#{pg_dump_result.command_line}' failed with #{pg_dump_result.exit_status}: stdout: #{pg_dump_result.output} stderr: #{pg_dump_result.error} ERROR raise ColumnOrderingError(err_message) end

completely agree, but I think it's out of scope for this PR

NickLaMuro · 2017-07-12T23:53:05Z

spec/spec_helper.rb

@@ -33,10 +33,10 @@ def image_path(path, options = {})
 Dir[ManageIQ::Gems::Pending.root.join("spec/support/custom_matchers/*.rb")].each { |f| require f }

 RSpec.configure do |config|
-  config.expect_with :rspec do |c|
+  config.expect_with(:rspec) do |c|


Confused at why we have changes to the spec/ dir, when you ignore it in ManageIQ/guides#228.

At first, I tried to include everything except for *_spec.rb, but that was too optimistic, so I changed the config to exclude spec/*, but didn't remove the changes from this PR yet.

NickLaMuro · 2017-07-12T23:54:52Z

lib/services/resource_sharer.rb

@@ -8,7 +8,7 @@ class ResourceSharer

  attr_accessor :user, :resource, :tenants, :features, :allow_tenant_inheritance

-  with_options :presence => true do
+  with_options(:presence => true) do


I would argue that this is a DSL type syntax, and should be left as is.

NickLaMuro · 2017-07-12T23:56:03Z

tools/configure_server_settings.rb

-  opt :serverid, "Server Id",                          :short => "s", :default => 0
-  opt :path,     "Path within advanced settings hash", :short => "p", :default => ""
-  opt :value,    "New value for setting",              :short => "v", :default => ""
+  opt(:dry_run,  "Dry Run",                            :short => "d")


I also think that these opt fall under the umbrella of "DSL"

Yes these are DSL, IMO

NickLaMuro · 2017-07-12T23:59:47Z

lib/extensions/ar_miq_set.rb

@@ -18,8 +18,8 @@ module ActsAsMiqSetMember
    include RelationshipMixin
    self.default_relationship_type = "membership"

-    alias_with_relationship_type :memberof,          :parents
-    alias_with_relationship_type :make_not_memberof, :remove_parent
+    alias_with_relationship_type(:memberof,          :parents)


I see all of these calls to alias_with_relationship_type as a DSL as well. Arguably this is one that we own (I think), and that would be a slippery slop of how much of our own DSLs we include in the blacklist in ManageIQ/guides#228 .

I agree, the intention here was for it to be a DSL.

NickLaMuro · 2017-07-13T00:02:56Z

config/routes.rb


  # Let's serve pictures directly from the DB
-  get '/pictures/:basename' => 'picture#show', :basename => /[\da-zA-Z]+\.[\da-zA-Z]+/
+  get('/pictures/:basename' => 'picture#show', :basename => /[\da-zA-Z]+\.[\da-zA-Z]+/)


Yes, the routes file is all DSL

NickLaMuro · 2017-07-13T00:05:50Z

app/models/vm/operations.rb

@@ -22,7 +22,7 @@ def cockpit_url
  end

  def ipv4_address
-    ipaddresses.find { |ip| (IPAddr.new ip).ipv4? }
+    ipaddresses.find { |ip| IPAddr.new(ip).ipv4? }


Just wanted to say that this is probably how this one should have been written in the first place.

NickLaMuro · 2017-07-13T00:11:40Z

app/models/miq_report_result.rb

@@ -6,7 +6,7 @@ class MiqReportResult < ApplicationRecord
  belongs_to :miq_task
  has_one    :binary_blob, :as => :resource, :dependent => :destroy
  has_many   :miq_report_result_details, :dependent => :delete_all
-  has_many   :html_details, -> { where "data_type = 'html'" }, :class_name => "MiqReportResultDetail", :foreign_key => "miq_report_result_id"
+  has_many   :html_details, -> { where("data_type = 'html'") }, :class_name => "MiqReportResultDetail", :foreign_key => "miq_report_result_id"


Why isn't this just using the hash syntax? This is already a problem, just don't get the reason for the string based .where here.

¯\_(ツ)_/¯

NickLaMuro · 2017-07-13T00:30:04Z

@bdunne Consider rebasing the last two commits you added prior to merging with the large one as they are just fixing up flubs from the auto-correct, and the correction commits just add noise when blaming in the future.

jrafanie · 2017-07-13T02:16:52Z

but didn't want to introduce a lot of code change here.

+1,118 −1,123

Too late!

😆

jrafanie · 2017-07-13T02:20:37Z

Wow. This was a lot of work. I don't know how to review it though.

NickLaMuro · 2017-07-13T02:31:07Z

Wow. This was a lot of work. I don't know how to review it though.

@jrafanie While it doesn't help with the "volume" of changes, the following is a way to at least view the diff as a whole without needing to press "Load Diff" for half of the files.

$ vim -n <(curl -Ls https://github.com/ManageIQ/manageiq/pull/15561.diff)

carbonin · 2017-07-13T13:55:38Z

I found a few others that seem DSL-ish to me and probably should be excluded.

throw
yield
module_function
define_callbacks
set_callback

miq-bot · 2017-07-14T13:12:05Z