Skip to content
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

Add a Ruby checker using Reek #1244

Merged
merged 1 commit into from Apr 28, 2017

Conversation

@Simplify
Copy link
Member

commented Apr 27, 2017

  • Added ruby-reek checker
  • Fixed Ruby related integration tests

.. note::

``flycheck-reekrc`` is default set to ``nil``, because Reek can find

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

is default set todefaults to or is set to … by default

flycheck.el Outdated
@@ -9,7 +9,7 @@
;; URL: http://www.flycheck.org
;; Keywords: convenience, languages, tools
;; Version: 31-cvs
;; Package-Requires: ((dash "2.12.1") (pkg-info "0.4") (let-alist "1.0.4") (seq "1.11") (emacs "24.3"))
;; Package-Requires: ((dash "2.12.1") (pkg-info "0.4") (let-alist "1.0.4") (seq "1.11") (emacs "24.4"))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

Ouch, must we ? 24.3 is still the default on Debian LTS.

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

Sorry, my bad. Got warning that several functions require 24.4. Just today I realized that those are also implemented here for users on 24.3.

flycheck.el Outdated

See URL `https://github.com/troessner/reek' for more information
about Reek."
(let ((errors))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

I prefer an explicit (errors nil) here.

flycheck.el Outdated
(let ((errors))
(dolist (message (car (flycheck-parse-json output)))
(let-alist message
(dolist (line (delete-dups .lines))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

Where is delete-dups from?

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

Manual :)
https://www.gnu.org/software/emacs/manual/html_node/elisp/Sets-And-Lists.html#Sets-And-Lists

Sometimes you can have same error twice on same line. Then you get output like this:

[{"context":"ApplicationController#validate_type","lines":[15,15,16],"message":"calls 'params['data']' 3 times","smell_type":"DuplicateMethodCall","source":"app/controllers/application_controller.rb","name":"params['data']","count":3,"wiki_link":"https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md"}]

Line 15 is two times there.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 28, 2017

Member

Thanks; for whatever reason I thought delete-dups was in `cl'

flycheck.el Outdated
(defun flycheck-parse-reek (output checker buffer)
"Parse Reek warnings from JSON OUTPUT.

CHECKER and BUFFER denoted the CHECKER that returned OUTPUT and

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

denoteddenote

flycheck.el Outdated

;; Default to `nil' to let Reek find its configuration file by itself
(flycheck-def-config-file-var flycheck-reekrc ruby-reek nil
:safe #'stringp

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

Isn't it a bit weird that the default value doesn't pass the safe predicate?

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

Changed to #'string-or-null-p. There are few places for other config files that have same problem.

@@ -8852,7 +8894,7 @@ current versions of MRI and JRuby, but may break when used with
other implementations or future versions of these
implementations.

Please consider using `ruby-rubocop' or `ruby-rubylint' instead.
Please consider using `ruby-rubocop' or `ruby-reek' instead.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

Why remove rubylint here?

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

ruby-lint is not actively maintained.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 28, 2017

Member

Ok, makes sense :)

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

I'm planing to take a look on Ruby checkers order again, when new chaining gets close to release.

'(5 7 error "unexpected token tCONSTANT" :checker ruby-rubocop)
'(5 24 error "unterminated string meets end of file" :checker ruby-rubocop)))
'(5 7 error "unexpected token tCONSTANT (Using Ruby 2.1 parser; configure using `TargetRubyVersion` parameter, under `AllCops`)" :checker ruby-rubocop)
'(5 24 error "unterminated string meets end of file (Using Ruby 2.1 parser; configure using `TargetRubyVersion` parameter, under `AllCops`)" :checker ruby-rubocop)))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

Thanks for updating these tests. Is there a way to suppress the version number in there to make them more robust?

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

Only if add configuration file for rubocop in repo. This will change a lot, no matter what we do. There is PR that fixes integration tests from about a month ago and testing output from rubocop is already outdated. rubocop is constantly under active development, many things change very fast.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 28, 2017

Member

Ok. A necessary evil, I guess.

(flycheck-error-new-at
line
nil
'warning (concat .context " " .message)

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 27, 2017

Member

Maybe use ": " instead of " "?

This comment has been minimized.

Copy link
@Simplify

Simplify Apr 28, 2017

Author Member

I was thinking about that, but decided not to do it. The reasons are:

  • This way warning is normal sentence
  • When requesting standard text output reek does it same way:
  [15, 15, 16]:DuplicateMethodCall: ApplicationController#validate_type calls 'params['data']' 3 times [https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md]

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 28, 2017

Member

Thanks, makes sense.

@Simplify Simplify force-pushed the ruby-reek branch from 5d0aee3 to 3d229dc Apr 28, 2017

@Simplify

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

@cpitclaudel this is ready for review again

@cpitclaudel
Copy link
Member

left a comment

Thanks :) Two minor tweaks, then you can go ahead and merge! Thanks for this excellent PR.

CHANGES.rst Outdated
@@ -14,6 +14,7 @@
- Dockerfile with ``hadolint`` [GH-1194]
- AsciiDoc with ``asciidoctor`` [GH-1167]
- CSS/SCSS/LESS with ``stylelint`` [GH-903]
- Ruby with ``reek``

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 28, 2017

Member

Please add a link to this PR on this line :)


.. note::

``flycheck-reekrc`` defaults to ``nil``, because Reek can find his own

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Apr 28, 2017

Member

hisits

@Simplify Simplify force-pushed the ruby-reek branch from 3d229dc to e67571e Apr 28, 2017

@Simplify Simplify merged commit 6354ecc into master Apr 28, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@Simplify Simplify deleted the ruby-reek branch Apr 28, 2017

@Simplify

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

@cpitclaudel Thanks a lot:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.