Add RuboCop testing #112

shortdudey123 · 2016-08-25T00:08:58Z

Adds rubocop to standardize syntax
Let me know if there is anything you don't agree with

mthssdrbrg · 2016-09-14T19:48:50Z

Rakefile

-          else
-            break
-          end
+          break unless answer.empty? || answer == 'y'


I usually refrain from using unless with multiple conditions as I think it's more difficult to read (I usually have to read an unless statement with multiple conditions multiple times before actually understanding it).

multiple conditions

I would agree is this was an if/elsif block, but its an if/else block so there is only one condition to chef, not multiple

Well, there are multiple conditions in the unless part answer.empty? OR answer == 'y', but for this one I'll actually consider the unless variant as it's easy to grip, so ignore my comment :).

Ah i gotcha

mthssdrbrg · 2016-09-14T19:49:06Z

libraries/log4j.rb

+                       %(#{prefix}.#{camelcase(key)}=#{value.call})
+                     else
+                       %(#{prefix}.#{camelcase(key)}=#{value})
+                     end


Personally, I think that the changed version is more difficult to read, and it hurts my eyes :)

Style/ConditionalAssignment can force the original with EnforcedStyle: assign_inside_condition if you would like :)

Yes please.

mthssdrbrg · 2016-09-14T19:50:41Z

Things I don't agree with:

Removal of trailing commas. I prefer them as they make diffs easier to read through when items are added to arrays and/or hashes.
%w() over %w[]. Using brackets makes it obvious (or at least more so) that one is dealing with an array than parentheses does.
backticks over %x(). This might be a weird one, but when writing bash scripts I use $() over backticks as backticks are considered legacy, see SC1077 and SC2006.
Spaces before/after {} for one-line hashes. Don't really see the benefit of adding spaces?
Opening/closing {} for method calls where they're actually not needed. Very much a personal preference without any real motivation for it, but I prefer to have them as it's appealing :).

I'd also prefer if each "rule" change had its own commit as it would make it easier to discard certain changes.

shortdudey123 · 2016-09-14T20:02:53Z

Removal of trailing commas. I prefer them as they make diffs easier to read through when items are added to arrays and/or hashes.

This was inconsistent throughout so i picked one, will force a trailing comma

%w() over %w[]

This will affect all % shortcuts, not just %w

backticks over %x()

The 2 links you mentioned are on the shell side, not ruby side

Spaces before/after {} for one-line hashes

forces consistency, i can force Style/SpaceBeforeBlockBraces either way

Opening/closing {} for method calls where they're actually not needed.

Can you elaborate on this? not following what you are referring too
Hash.new vs {}?

I'd also prefer if each "rule" change had its own commit as it would make it easier to discard certain changes.

Thought about it, but sometimes a line is affected by several rules so its hard to revert a commit in the middle

mthssdrbrg · 2016-09-14T20:30:55Z

This was inconsistent throughout so i picked one, will force a trailing comma

Ah :)

This will affect all % shortcuts, not just %w

That's unfortunate :/.

The 2 links you mentioned are on the shell side, not ruby side.

I know, which is why I said it was weird :). I don't agree with the motivation for using %x() though, and would prefer %x|<command>| over <command>.

forces consistency, i can force Style/SpaceBeforeBlockBraces either way

Not really sure what this means? Is there a rule for forcing spaces before block braces but not for hashes? If so that sounds fine.

Can you elaborate on this? not following what you are referring too
Hash.new vs{}?

Sure. What I was referring to was method calls as:

opts[:env_path] = value_for_platform_family({
  'debian' => '/etc/default/kafka',
   'default' => '/etc/sysconfig/kafka',
})

Whereas the rubocop rules suggests that the {} should be stripped. Does that make sense?

shortdudey123 · 2016-09-14T21:18:09Z

That's unfortunate :/.

scratch that... looks like Style/PercentLiteralDelimiters allows config per % shortcut, force %w to be []? or stay consistent with everything else with ()

I know, which is why I said it was weird :). I don't agree with the motivation for using %x() though, and would prefer %x|| over <command>.

will look more at this

Not really sure what this means? Is there a rule for forcing spaces before block braces but not for hashes? If so that sounds fine.

I can force a space or no space, (actually the cop is Style/SpaceInsideBlockBraces)
Will look more if i can only force inside blocks and not hashes

Whereas the rubocop rules suggests that the {} should be stripped. Does that make sense?

Gotcha, this is Style/BracesAroundHashParameters. It suggests removing them since they are redundant.

mthssdrbrg · 2016-09-15T16:24:04Z

scratch that... looks like Style/PercentLiteralDelimiters allows config per % shortcut, force %w to be[]? or stay consistent with everything else with ()

Right now I'm leaning towards having anything that creates arrays/lists use [] and use () for the rest. Including %x(), so there doesn't have to be some weird special case for that :).

Gotcha, this is Style/BracesAroundHashParameters. It suggests removing them since they are redundant.

Yeah. On second thought I don't really have a strong preference for this, so it's okay to include that rule (i.e. remove the redundant braces).

shortdudey123 · 2016-09-15T17:18:39Z

Cool, thanks for all the feedback! i will work on updates today

shortdudey123 · 2016-09-16T01:36:35Z

The the spaces after { and before } i can do a couple different things

Force no spaces for both hashes and block params
Force no spaces for hashes and no space after { in a block but a space before } in a block

The way the SpaceInsideBlockBraces cop works, is no space for everything, or spaces for everything except the { of a block

shortdudey123 · 2016-09-16T01:37:11Z

Besides the formatting in the last comment, i think i addressed the feedback

shortdudey123 · 2016-10-07T21:46:06Z

@mthssdrbrg any further thoughts?

mthssdrbrg

Looks good in general, but there are some inconsistencies especially with trailing commas, that rubocop doesn't complain about when I run it locally (which is really weird...).

mthssdrbrg · 2016-10-09T14:18:12Z

libraries/log4j.rb

+                       %(#{prefix}.#{camelcase(key)}=#{value.call})
+                     else
+                       %(#{prefix}.#{camelcase(key)}=#{value})
+                     end


Yes please.

mthssdrbrg · 2016-10-09T14:18:31Z

recipes/_service.rb

    daemon_name: 'kafka',
    port: node['kafka']['broker']['port'],
    user: node['kafka']['user'],
    env_path: kafka_init_opts[:env_path],
    ulimit: node['kafka']['ulimit_file'],
-    kill_timeout: node['kafka']['kill_timeout'],
-  })
+    kill_timeout: node['kafka']['kill_timeout']


What happened to the trailing comma here?

not needed per Style/TrailingCommaInArguments

mthssdrbrg · 2016-10-09T14:18:57Z

spec/recipes/configure_spec.rb

        owner: 'root',
        group: 'root',
        mode: script_permissions,
-        source: source_template,
-      })
+        source: source_template


Same with trailing comma here.

not needed per Style/TrailingCommaInArguments

mthssdrbrg · 2016-10-09T14:20:11Z

spec/recipes/install_spec.rb

-    })
+    expect(chef_run).to run_execute('extract-kafka').with(
+      cwd: %(#{Dir.tmpdir}/kafka-build)
+    )


Think this could be collapsed to a single line, i.e.

expect(chef_run).to run_execute('extract-kafka').with(cwd: %(#{Dir.tmpdir}/kafka-build))

currently, this is consistent style
I can make it inconsistent if you prefer

mthssdrbrg · 2016-10-09T14:21:46Z

Rakefile

  cookbook = Stove::Cookbook.new(Dir.pwd)
  version = cookbook.tag_version
  release_name = %(kafka-cookbook-#{version})
-  archive_path = ::File.join('pkg', sprintf('kafka-cookbook-%s.tar.gz', version))
+  archive_path = ::File.join('pkg', "#{release_name}.tar.gz")


Would prefer if this was switched out to a format(...).

mthssdrbrg · 2016-10-09T14:22:45Z

libraries/helpers.rb

        'default' => 'systemd/default.erb'
-      })
+      )


There are quite some inconsistencies in this method with regards to trailing commas, though rubocop doesn't complain when I check this file locally which I think is quite weird?

trailing comma only applies to multiline hashes, not method calls

let me know if you want Style/TrailingCommaInArguments disabled anyway

shortdudey123 · 2016-10-09T23:58:26Z

addressed all feedback

shortdudey123 · 2016-10-27T06:38:20Z

You want me to squash the commits or are you good with the history?

mthssdrbrg · 2016-10-27T07:54:09Z

I'm good with the history, and I was hoping to merge this yesterday but I wanted to go through the changes one more time but ran out of time, will get to it tonight.

Thanks a lot for the contribution (and your patience!) @shortdudey123.

shortdudey123 · 2016-10-27T08:10:18Z

Cool and no problem! :)

mthssdrbrg · 2016-10-27T19:20:28Z

Merged in 584b40e.

lock · 2020-04-25T09:10:02Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mthssdrbrg reviewed Sep 14, 2016

View reviewed changes

shortdudey123 added 2 commits September 15, 2016 17:12

Add RuboCop testing

4034e71

Force trailing comma on multiline hashes and arrays

d2a5b09

shortdudey123 force-pushed the add_rubocop branch from 1f8d259 to d2a5b09 Compare September 16, 2016 00:24

Force %w to use [] not ()

36326c5

mthssdrbrg suggested changes Oct 9, 2016

View reviewed changes

shortdudey123 added 2 commits October 9, 2016 16:47

interpolation to format

f8ae28e

Update Style/ConditionalAssignment settings

fc46302

mthssdrbrg approved these changes Oct 26, 2016

View reviewed changes

mthssdrbrg closed this Oct 27, 2016

shortdudey123 deleted the add_rubocop branch October 27, 2016 19:27

lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RuboCop testing #112

Add RuboCop testing #112

shortdudey123 commented Aug 25, 2016

mthssdrbrg Sep 14, 2016

shortdudey123 Sep 14, 2016

mthssdrbrg Sep 14, 2016

shortdudey123 Sep 14, 2016

mthssdrbrg Sep 14, 2016

shortdudey123 Sep 14, 2016

mthssdrbrg Oct 9, 2016

mthssdrbrg commented Sep 14, 2016

shortdudey123 commented Sep 14, 2016

mthssdrbrg commented Sep 14, 2016

shortdudey123 commented Sep 14, 2016

mthssdrbrg commented Sep 15, 2016

shortdudey123 commented Sep 15, 2016

shortdudey123 commented Sep 16, 2016

shortdudey123 commented Sep 16, 2016

shortdudey123 commented Oct 7, 2016

mthssdrbrg left a comment

mthssdrbrg Oct 9, 2016

mthssdrbrg Oct 9, 2016

shortdudey123 Oct 9, 2016

mthssdrbrg Oct 9, 2016

shortdudey123 Oct 9, 2016

mthssdrbrg Oct 9, 2016

shortdudey123 Oct 9, 2016

mthssdrbrg Oct 9, 2016

mthssdrbrg Oct 9, 2016 •

edited

Loading

shortdudey123 Oct 9, 2016 •

edited

Loading

shortdudey123 Oct 9, 2016

shortdudey123 commented Oct 9, 2016

shortdudey123 commented Oct 27, 2016

mthssdrbrg commented Oct 27, 2016

shortdudey123 commented Oct 27, 2016

mthssdrbrg commented Oct 27, 2016

lock bot commented Apr 25, 2020

Add RuboCop testing #112

Add RuboCop testing #112

Conversation

shortdudey123 commented Aug 25, 2016

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

mthssdrbrg commented Sep 14, 2016

shortdudey123 commented Sep 14, 2016

mthssdrbrg commented Sep 14, 2016

shortdudey123 commented Sep 14, 2016

mthssdrbrg commented Sep 15, 2016

shortdudey123 commented Sep 15, 2016

shortdudey123 commented Sep 16, 2016

shortdudey123 commented Sep 16, 2016

shortdudey123 commented Oct 7, 2016

mthssdrbrg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthssdrbrg Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

shortdudey123 Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shortdudey123 commented Oct 9, 2016

shortdudey123 commented Oct 27, 2016

mthssdrbrg commented Oct 27, 2016

shortdudey123 commented Oct 27, 2016

mthssdrbrg commented Oct 27, 2016

lock bot commented Apr 25, 2020

mthssdrbrg Oct 9, 2016 •

edited

Loading

shortdudey123 Oct 9, 2016 •

edited

Loading