New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Rubocop for coding style checks #177

Merged
merged 80 commits into from Dec 11, 2014

Conversation

Projects
None yet
4 participants
@lslezak
Copy link
Member

lslezak commented Nov 24, 2014

  • Generated default config by rubocop --auto-gen-config
  • Fixed each issue in a separate commit and enabled the check (many issues were auto corrected using --auto-correct Rubocop option)
  • Added rubocop call at Travis

Submitting to SLE-12 branch to avoid conflicts in the future and make maintenance easier.

lslezak added some commits Nov 19, 2014

@lslezak lslezak force-pushed the rubocop branch from 8a79a32 to 61316d5 Nov 26, 2014

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 26, 2014

Coverage Status

Coverage increased (+0.07%) when pulling 61316d5 on rubocop into e8dfefe on SLE-12-GA.

@lslezak

This comment has been minimized.

Copy link
Member

lslezak commented Nov 27, 2014

PR updated - removed function renames (not desired in SLE12) and fixed the issues reported at yast2-devel@

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 27, 2014

Coverage Status

Coverage increased (+0.07%) when pulling d6bf450 on rubocop into e8dfefe on SLE-12-GA.

mvidner added some commits Dec 11, 2014

rubocop: separated a common YaST style
Same content as for nfs-client
Once things stabilize it should move to devtools/buildtools.
rubocop: remove "Style/MultilineOperationIndentation" from the common…
… part

See also 3cbfa31
This commit makes Rubocop pass again while highlighting the difference
in style to nfs-client.
Hopefully we can agree on a common style soon.
@mvidner

This comment has been minimized.

Copy link
Member

mvidner commented Dec 11, 2014

I have factored out .rubocop_yast_style.yml with the intent of eventually moving it to a base package. But I found out that we differ in Style/MultilineOperationIndentation. In particular I quite dislike non-indenting the condition continuation:

      addon = Addon.find_all(self).find do |a|
        a.arch == remote_product.arch &&
        a.identifier == remote_product.identifier &&
        a.version  == remote_product.version &&
        a.release_type == remote_product.release_type
      end

but I am willing to change my mind. For indentation style I think editor support matters. I'll look at this in Emacs and vim.

@mvidner

This comment has been minimized.

Copy link
Member

mvidner commented Dec 11, 2014

Oh nice, Travis fails beacuse of new checks introduced by Rubocop 0.28.0.

@mvidner

This comment has been minimized.

Copy link
Member

mvidner commented Dec 11, 2014

I think we must not depend on bleeding edge Rubocop, instead pinning it to some version now, and only adopt new checks once in a while, to reduce constant distractions.

rubocop pinned to version ~> 0.27.0
0.28.0 detects "Extra empty line detected at block body beginning."
which I don't want to deal with now. Or with Style/WhateverNewCheck
appears in 0.29.0.
@@ -315,7 +315,7 @@ def update_base_product
# @yieldreturn [Boolean, SUSE::Connect::Remote::Product] success flag and
# remote product pair
# @return [Boolean] true on success
def handle_product_service(&block)
def handle_product_service(&_block)

This comment has been minimized.

@mvidner

mvidner Dec 11, 2014

Member

This is wrong! If you look only at the signature you will think that the block is never called. In fact it is called via yield, which should be changed to block.call instead.

@@ -54,7 +53,7 @@ class ConnectHelpers
# @param message_prefix [String] Prefix before error like affected product or addon
# @param show_update_hint [Boolean] true if an extra hint for registration update
# should be displayed
def self.catch_registration_errors(message_prefix: "", show_update_hint: false, &block)
def self.catch_registration_errors(message_prefix: "", show_update_hint: false, &_block)

This comment has been minimized.

@mvidner

mvidner Dec 11, 2014

Member

s/yield/block.call/

:version => product["version"],
:release_type => product["release_type"]
) : product
def service_for_product(product, &_block)

This comment has been minimized.

@mvidner

mvidner Dec 11, 2014

Member

block.call

This comment has been minimized.

@mvidner

mvidner Dec 11, 2014

Member

Also, the parameter is not documented, which may have played a role in the misguided "fix".

@@ -338,7 +338,7 @@ def self.update_product_renames(renames)
# a helper method for iterating over repositories
# @param repo_aliases [Array<String>] list of repository aliases
# @param block block evaluated for each found repository
def self.each_repo(repo_aliases, &block)
def self.each_repo(repo_aliases, &_block)

This comment has been minimized.

@mvidner

mvidner Dec 11, 2014

Member

block.call


# user pressed the "Import" button
expect(Yast::UI).to receive(:UserInput).and_return(:import)

# check the displayed content
expect(Yast::UI).to receive(:OpenDialog) do |arg1, arg2|
expect(Yast::UI).to receive(:OpenDialog) do |_arg1, arg2|

This comment has been minimized.

@mvidner

mvidner Dec 11, 2014

Member

better |_opt, content|

@mvidner

This comment has been minimized.

Copy link
Member

mvidner commented Dec 11, 2014

After my changes it LGTM :-) I hope they were for the better. Thanks for starting this!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 11, 2014

Coverage Status

Coverage increased (+0.07%) when pulling 338e5cf on rubocop into e8dfefe on SLE-12-GA.

lslezak added a commit that referenced this pull request Dec 11, 2014

Merge pull request #177 from yast/rubocop
Use Rubocop for coding style checks

@lslezak lslezak merged commit 8c22de8 into SLE-12-GA Dec 11, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@lslezak lslezak deleted the rubocop branch Dec 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment