troessner / reek

Code smell detector for Ruby
https://github.com/troessner/reek
MIT License
4k stars 279 forks source link

Support Ruby 3.1 in Code Climate engine #1687

Closed dantevvp closed 6 months ago

dantevvp commented 1 year ago

Hello from Code Climate! We're looking to update the codeclimate-reek engine to support parsing 3.1 syntax. Currently, the Dockerfile has ruby version 2.6 installed, so the Parser::CurrentRuby always picks Ruby26 as the parser, because that's the installed ruby version. This prevents reek from successfully reporting issues to Code Climate for files that include ruby 3.1 syntax.

Since reek always uses the parser corresponding to the installed ruby version, using ruby 3.1 as the base image would be my first suggestion. However, for better compatibility, we could also make the engine use the correct parser for every code base, by allowing a custom configuration attribute in .codeclimate.yml that lets the user set which ruby version they want reek to parse, like so:

#.codeclimate.yml

engines:
  reek:
    enabled: true
    config:
      target_ruby_version: "3.1" # override the default parser with Parser::Ruby31

I'd love to read your thoughts on this. Thanks!

mvz commented 1 year ago

Given how Reek works, I think there won't be any issue with just using the parser for the latest Ruby syntax.

dantevvp commented 1 year ago

Given how Reek works, I think there won't be any issue with just using the parser for the latest Ruby syntax.

@mvz Is the latest ruby parser always backwards compatible with older versions supported by Reek? If that's the case, then updating the Dockerfile's base image might just be enough!

mvz commented 1 year ago

I am not aware of any syntax that is valid for older supported Rubies but considered a syntax error for Ruby 3.1. If there is any, then that could be an issue.

dantevvp commented 1 year ago

I am not aware of any syntax that is valid for older supported Rubies but considered a syntax error for Ruby 3.1. If there is any, then that could be an issue.

I haven't found known cases of the 3.1 parser failing to parse older Ruby code, but after reading this section of the parser gem's README and knowing that this warning is shown whenever Parser::CurrentRuby is used with a different version than the one it supports (I.E. Ruby31 supports version 3.1.3, but you have version 3.1.2 installed), it leads me to think that using the parser for the latest Ruby syntax could cause some issues in the long run. I believe that allowing Code Climate users to choose their target ruby version could make codeclimate-reek support Ruby 3.1 syntax while making the implementation future-proof.

If this were to be done, my idea would be to keep the change in the code_climate_reek file, overriding the Reek::Source::SourceCode class like so:

# bin/code_climate_reek

module Reek
  module Source
    class SourceCode
      # override self.default_parser method
      def self.default_parser
        parser_class.new(AST::Builder.new).tap do |parser|
          diagnostics = parser.diagnostics
          diagnostics.all_errors_are_fatal = true
          diagnostics.ignore_warnings      = true
        end
      end

      # config.json file will look like this:
      # {
      #   "include_paths":[
      #     "lib",
      #     "spec"
      #   ],
      #   "config": {
      #     "target_ruby_version": "3.1.0"
      #   }
      # }
      def self.parser_class
        # convert an X.Y.Z version number to an XY two digit number
        version_number = CodeClimateToReek.new.target_ruby_version&.gsub(/(\d+)\.(\d+)(\..+)?/, '\1\2').to_i

        return Parser::CurrentRuby if version_number.zero?

        Reek::CLI::Silencer.silently { require "parser/ruby#{version_number}" }
        Module.const_get("Parser::Ruby#{version_number}")
        end
      rescue LoadError, NameError
        # use Parser::CurrentRuby when an invalid version number is provided
        Parser::CurrentRuby
      end
    end
  end
end

@mvz Of course, feel free to comment on this and let me know what you think, there possibly might be a better solution to this that I haven't seen or thought of.

mvz commented 1 year ago

@dantevvp something like that seems fine. Until we decide to support such a setting in reek itself, code_climate_reek is the best place for this code to live.

@troessner any thoughts on this?

troessner commented 1 year ago

Looks good to me @mvz, @dantevvp - looking forward to the pull request ;)

dantevvp commented 1 year ago

Cool, many thanks for the responses! I opened a PR at #1694

bestwebua commented 1 year ago

@dantevvp Hola, Dante! Aslo it would be great if somebody will add ability to configure custom location of Reek configuration file:

engines:
  reek:
    enabled: true
    config:
      file: linter_configs/.reek.yml

I understend it's outside of scope of this issue, I have asked here: https://github.com/codeclimate/codeclimate/issues/1078, but just silence...

mvz commented 6 months ago

Since #1694 has been merged and released, I'm closing this issue.