-
Notifications
You must be signed in to change notification settings - Fork 280
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
Code Climate: add config "target_ruby_version" to support different versions of Ruby syntax #1694
Code Climate: add config "target_ruby_version" to support different versions of Ruby syntax #1694
Conversation
Thanks, @dantevvp, I'll try to do a review this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine. I have just two suggestions for improvement.
bin/code_climate_reek
Outdated
module Reek | ||
module Source | ||
# Override Reek::Source::SourceCode to use a parser version specified by the user | ||
class SourceCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other overrides in this file are done using Module#prepend
. I think it makes sense to do that here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
@dantevvp I'm assuming you will test that this works in the context of Code Climate yourself 🙂 |
Correct! I've tested this with 3.1 syntax and old syntax. It defaults to |
I'm not sure if we need to release a new version. Doesn't Code Climate run reek straight from the repository using the Dockerfile? @dantevvp would know this ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good as soon as rubocop is happy 😉
To let users choose what Ruby version they want to use reek under, expose a new config attribute "target_ruby_version". This overrides the Reek::Source::SourceCode.default_parser method to use whichever parser version the user specifies Parse target_ruby_version with Gem::Version Co-authored-by: Matijs van Zuijlen <[email protected]> Use prepend to override SourceCode class methods remove trailing whitespace
20ae1f2
to
b74920e
Compare
@troessner @mvz Commits squashed and Rubucop offense resolved!
This is correct! We use the commit sha of the repository so a version release would not be necessary to get the latest Reek code. |
Thanks, @dantevvp! |
This plugin has been failing for years because Code Climate insists on running reek under 2.6 with parser 3.1. Today, I found troessner/reek#1694, which shows an undocumented way to set a target version for reek. This commit sets a target version of 3.1 and fixes some of the "new" findings.
Related issue: #1687
Currently, Reek uses
Parser::CurrentRuby
to determine which Ruby version to parse when analyzing code, so it always chooses the installed Ruby version's parser. Under the Code Climate engine, this means that it will always choose the parser for Ruby version2.6
, because that's the version that is installed in theDockerfile
.To let users choose what Ruby version they want to use reek under, expose a new config attribute "target_ruby_version". This overrides the Reek::Source::SourceCode.default_parser method to use whichever parser version the user specifies.
Example
With
.codeclimate.yml
looking like this:The logic will use the first two digits of the chosen version to determine which parser version to use. In this case, it would be
Parser::Ruby26
Another example:
In this case, we will choose
Parser::Ruby31
as the parser, which supports newer syntax.If the user specifies an unknown version or no version at all, it will default to
Parser::CurrentRuby
, which is the previous behavior.