Static code analysis via Rubocop #857

klappradla · 2017-09-16T21:57:10Z

Related to the discussion in #845, this adds static code analysis via Rubocop as an additional step in the CI pipeline (a super fast step though, no worries).

Please see the commit history and the .rubocop.yml file in this PR for the details on which cops are enabled and which I already ran.
Rubocop has a large list of per-default enabled cops. Since the number of cops we want to have enabled is rather small, I instead of manually disabling the ones we don't want, disabled all and just added some specific ones. The file is rather large, since I also included the description and link to the styleguide for each cop. I thought it could be more helpful for newcomers or people coming to the list from the contribution guide.

From @carpodaster's initial list of cops, I for now skipped everything related to specs since it would require another gem (I can do this once we agreed on the setup for the app code) and did not enable the one for line length and the frozen string literal comment, since this would basically touch every file in the repo 🙈 (I'd then also do this in a separate PR).

Aside that, I also enabled some very basic cops related to things like trailing whitespaces, indentation, etc. (see the file and the commit history for details).

I also update the Readme and Contribution guide to include all this ☝️

…s-via-rubocop-codeclimate

Add instructions for running code analyzers and some general cleanup.

[ci skip]

emcoding · 2017-09-16T22:36:40Z

Wow Max, that is an amazing job! Thank you!! 🎁
My future self is already immensely grateful. 😛

I feel you added some great cops (re: Ruby/Rails conventions), and also some that are probably up for debate; for instance one that I don't see as an important thing, and even one that I don't like at all 🙊
I don't name them, because my concern is: how are we going to prevent lengthy discussions about the extra cops? Especially because that was one of the concerns when we talked about introducing
Rubocop in the first place.
So, is it okay if I just review the code, and refrain from adding opinions on the cops itself? Or did you actually hope to get second opinions? Because then we should think of something to address that.

Either way, I am really stoked about getting Rubocop. Dev Happiness Score: 💯

klappradla · 2017-09-16T22:52:12Z

Well, I neither have any expectations, nor do I share your concerns @F3PiX. So feel free to comment as you wish ✌️

emcoding

Again, big thanks Max!
I added some suggestions, mostly for some details in descriptions.
And I added a lean 'votes' system for the cops you introduced, either a 😍 for cops I really love, or a 😢 for things I'd rather leave out.

emcoding · 2017-09-17T10:32:07Z

.rubocop.yml

+Naming/ClassAndModuleCamelCase:
+  Description: "Use CamelCase for classes and modules."
+  StyleGuide: "#camelcase-classes"
+  Enabled: true


emcoding · 2017-09-17T10:32:25Z

.rubocop.yml

+  Description: "Constants should use SCREAMING_SNAKE_CASE."
+  StyleGuide: "#screaming-snake-case"
+  Enabled: true
+


emcoding · 2017-09-17T10:33:00Z

.rubocop.yml

+  Description: "Checks for trailing comma in argument lists."
+  StyleGuide: "#no-trailing-params-comma"
+  Enabled: true
+


Sorry, I don't understand your concern about this cop. What's the benefit of

def foo(bar,) # do stuff end

over

def foo(bar) # do stuff end

???

You can run rubocop -a and it will automatically remove these commas for you.

Maybe I misunderstood. I thought it were about the kind of comma's at the end of this line https://github.com/rails-girls-summer-of-code/rgsoc-teams/pull/857/files#diff-ea406fc966c043b0342994808d837c01L65
Of course I don't weep about the example you give above 😂

emcoding · 2017-09-17T10:37:32Z

app/controllers/roles_controller.rb

@@ -57,7 +57,7 @@ def set_team

    def set_role
      @role = if params[:id]
-        @team.roles.find(params[:id])
+                @team.roles.find(params[:id])
      else


Is this alignment a cop rule or a typo? (also in next 2 files)

OTOH this for example:
https://github.com/rails-girls-summer-of-code/rgsoc-teams/pull/857/files#diff-2792d18537f304cd4f8aeee92193583dR11
Cops love Space 👾

The 2 spaces indentation is based on the if... The "normal" way of formatting this would be:

@role = if params[:id] @team.bla else # other end

But I did not want to enforce this via another cop, so it looks kinda weird here. I can manually change it to the above.

emcoding · 2017-09-17T10:37:46Z

app/controllers/sources_controller.rb

@@ -60,7 +60,7 @@ def set_team

    def set_sources
      @sources = if @team
-        @team.sources
+                   @team.sources
      else


☝️ = see above

emcoding · 2017-09-17T14:38:28Z

.rubocop.yml

+  Enabled: true
+
+Layout/CaseIndentation:
+  Description: "Indentation of when in a case/when/[else/]end."


Maybe include in the description, something like: align when/else/end/ with case. ?
To make it easier to understand what we are suppose to do about it.

I in 99% of cases just directly copied over the description from Rubocop's internal default rule set. Not sure if it makes sense to tweak them and do a lot of changes to them. For most cases the thing that helps explaining / understanding these rules best are examples - and they can be found in the Rubocop docs or the styleguide (also referenced for each rule).

So my idea was to keep the task of explaining things to the part the knows it best - naming the docs / styleguide - and not try to explain it in detail here.

But yeah, I still can do it if you think there's a huge benefit in having it.

Sure, let's leave it at the documentation! 👍

emcoding · 2017-09-17T14:44:28Z

.rubocop.yml

+  Enabled: false
+
+Style/HashSyntax:
+  Description: "Prefer Ruby 1.9 hash syntax { a: 1, b: 2 } over 1.8 syntax"


Maybe add a beginner friendly { :a => 1, :b => 2 } after 1.8 syntax in the description?

emcoding · 2017-09-17T14:44:57Z

.rubocop.yml

+  Enabled: true
+  Include:
+    - "**/Gemfile"
+    - "**/gems.rb"


👍 Love that we cover this already 👯

I had to google what gems.rb is all about. Good stuff, looking forward to it!

emcoding · 2017-09-17T14:45:22Z

.rubocop.yml

+
+Bundler/DuplicatedGem:
+  Description: "Checks for duplicate gem entries in Gemfile."
+  Enabled: true


Oh, that's a good one, yes

emcoding · 2017-09-17T14:51:26Z

.rubocop.yml

+AllCops:
+  TargetRubyVersion: 2.4
+  DisabledByDefault: true
+


Are there any files or directories that we should exclude? I know the db/schema.rb is sometimes excluded, but I don't know why TBH.

You're right, normally one would exclude e.g. the db and bin directories, just because the code there is not really regarded as "app code" and linting it often times does not make too much sense.

In our case, since we disable all cops, this is not really a problem for now. We don't have any cops that could be problematic for the schema file, migrations or the bin directory.

Aha 💡 . Thanks 🙇

This reverts commit 8034f74.

emcoding · 2017-09-17T19:35:04Z

How about we set up CodeClimate ? That was my initial idea for Rubocop. It is free for Open Source.

carpodaster

That's a job extremely well done – ¡Muchas gracias, señor! 💚 💜 ❤️

I added a few comments, but I'm also totally down to merge this as is.

carpodaster · 2017-09-17T20:21:18Z

.rubocop.yml

+  Enabled: true
+  Include:
+    - "**/Gemfile"
+    - "**/gems.rb"


I had to google what gems.rb is all about. Good stuff, looking forward to it!

carpodaster · 2017-09-17T20:22:15Z

CONTRIBUTING.md

@@ -94,7 +92,8 @@ If you've cloned the app a while ago, you want to make sure that your cloned `ma

 ### Write Code

-We aim to follow the [Ruby Style Guide](https://github.com/bbatsov/ruby-style-guide) but we don't enforce it (i.e. there is no [RuboCop](https://github.com/bbatsov/rubocop)). The most important thing is probably just proper indentation: two whitespaces.
+We aim to follow the [Ruby Style Guide](https://github.com/bbatsov/ruby-style-guide) and have  [RuboCop](https://github.com/bbatsov/rubocop) set up to make sure we stay consistent with some basic rules (see our [rubocop.yml](https://github.com/rails-girls-summer-of-code/rgsoc-teams/blob/master/.rubocop.yml) file for details).  
+But don't be afraid, the most important thing is probably just proper indentation: two whitespaces ✌️


👍 to @F3PiX's suggestion

carpodaster · 2017-09-17T20:23:25Z

app/controllers/conference_attendances_controller.rb

@@ -11,6 +11,7 @@ def update
  end

  private
+
    def attendance_params


Shouldn't this extra level of indentation be caught by RC?

Yes, but it's a different cop we don't yet have. But if we already have the one for a blank line before access modifiers and its indentation, it just makes sense to also add the one for proper (in my opinion 😉) indentation after it as well.

I'll add it ✌️

carpodaster · 2017-09-17T20:26:55Z

.rubocop.yml

+
+# TODO
+Metrics/LineLength:
+  Description: "Limit lines to 80 characters."


I know it's flagged as TODO and disabled, but should we up the limit to 100-something?

Maybe 120? Not sure what really makes sense... Currently we have 215 that's why it's a bit of an effort and I did not want to run it right away.

carpodaster · 2017-09-17T20:27:34Z

.rubocop.yml

+
+Bundler/DuplicatedGem:
+  Description: "Checks for duplicate gem entries in Gemfile."
+  Enabled: true


Oh, that's a good one, yes

carpodaster · 2017-09-17T20:32:04Z

.travis.yml

@@ -17,6 +17,7 @@ before_script:
 - npm install -g jshint
 script:
 - bundle exec rake spec
+- bundle exec rubocop


I wonder if we should use CodeClimate to run the cops; IMHO it gives better feedback on Github PRs: you get two CI results. Travis may run the specs just fine, it may "just" be coding style that needs improvement.

I have never set up CodeClimate with custom rules, though. If we cannot reuse our rubocop.yml, it's not worth pursuing this.

I think it works like this: we add a codeclimate.yml where we enable rubocop (and a js linter), and then cc uses our rubocop.yml for the rules.

If it is that easy, I'd prefer CC over running it on Travis

Just for the sake of completeness: I just came across build stages for Travis. Would also solve the problem I think.

I'll make a separate issue to discuss/decide on the CodeClimate way. Get it out of the way of merging this. 🎉

carpodaster · 2017-09-17T20:35:35Z

app/models/student.rb

@@ -43,7 +43,7 @@ def current_team

  def current_drafts
    @current_drafts ||= if current_team
-      current_team.application_drafts.in_current_season
+                          current_team.application_drafts.in_current_season


I think it is, but I cannot name it. I really like this way of indentation. Vim's Ruby syntax auto-formats blocks like this in exactly the same way.

Also: remove weird spec file that did not do anything.

[ci skip]

Switch pipeline order: lint before tests close 845

emcoding · 2017-09-18T20:56:45Z

🎉 🎉 🎉

klappradla added 27 commits September 14, 2017 23:10

Add rubocop

4926435

Autogenerate rubocop todo yml file

54b27c9

Add rubocop step to CI

a37ee65

Add target version to rubocop config

0bee768

Fix indentation of "private" macro

3938f63

Disable all cops by default 👮

595e15e

Rubocop: use 1.9 hash syntax

21bd663

Rubocop add cop for 2 spaces indent

c7508b6

Add cop for case-when indent

99d9942

Add some more style cops but don't enable them yet

44671fd

Merge branch 'master' into feature/845-discussion-static-code-analysi…

bf52042

…s-via-rubocop-codeclimate

Add and run cop for trailing whitespaces

8537cce

Add cop for redundant return statements

f396890

Add and run cop for redundant "self"s

cad6e49

Add and run cop for redundant parenthesis on method calls

9a5c6a0

Add and run cops for method def parenthesis

01b3803

Add descriptions to cops

7f05962

Add and run cop for blank line around access modifier

2fbcb50

Add and run cop to remove random spaces 🚀

ed8e029

Add and run rubocop for space before comments

2125af9

Add and run cops for spaces between comma, etc.

8194cff

Add and run cops for single blank line at the end of files

cee0d4e

Add and run cops for class and constant naming

4693d8a

Add and run cop to not unnecessarily nest else in if

e014cae

Update setup instructions in Readme

2a5cb19

Add instructions for running code analyzers and some general cleanup.

Update contribution guide to include info about code analyzers

270594a

[ci skip]

Remove rubocop todo-yml

87142a0

[ci skip]

klappradla requested review from carpodaster, juuh42dias and emcoding September 16, 2017 21:57

klappradla requested a review from branquinhoaa September 16, 2017 21:57

emcoding reviewed Sep 17, 2017

View reviewed changes

klappradla added 3 commits September 17, 2017 20:58

Revert accidental space-issue

8034f74

Revert "Revert accidental space-issue"

14494c7

This reverts commit 8034f74.

Fix alignment of if-else in roles_controller

3a8016b

carpodaster reviewed Sep 17, 2017

View reviewed changes

klappradla added 3 commits September 18, 2017 00:01

Add and run cops for consistent indentation

c92e3b8

Also: remove weird spec file that did not do anything.

Add and run cops for if-else alignment

b9f2403

Rephrase change in the contribution guide

9cf07cb

[ci skip]

emcoding mentioned this pull request Sep 18, 2017

How to Rubocop: CodeClimate, Travis or ...? #858

Closed

klappradla added 2 commits September 18, 2017 22:33

Update rubocop docs with 1.8 hash example

1a05d56

[ci skip]

Update travis.yml

09aa0e9

Switch pipeline order: lint before tests close 845

klappradla force-pushed the feature/845-discussion-static-code-analysis-via-rubocop-codeclimate branch from 7ae7d7b to 09aa0e9 Compare September 18, 2017 20:39

klappradla merged commit db178cf into master Sep 18, 2017

klappradla deleted the feature/845-discussion-static-code-analysis-via-rubocop-codeclimate branch September 18, 2017 20:43

Static code analysis via Rubocop #857

Static code analysis via Rubocop #857

Conversation

klappradla commented Sep 16, 2017

emcoding commented Sep 16, 2017

klappradla commented Sep 16, 2017

emcoding 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

klappradla Sep 17, 2017 • edited Loading

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

emcoding commented Sep 17, 2017

carpodaster 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

klappradla Sep 17, 2017 • edited Loading

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

emcoding commented Sep 18, 2017

klappradla Sep 17, 2017 •

edited

Loading

klappradla Sep 17, 2017 •

edited

Loading