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

Support Ruby 3.1 in Code Climate engine #1687

Closed
dantevvp opened this issue Dec 28, 2022 · 9 comments
Closed

Support Ruby 3.1 in Code Climate engine #1687

dantevvp opened this issue Dec 28, 2022 · 9 comments

Comments

@dantevvp
Copy link
Contributor

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
Copy link
Collaborator

mvz commented Dec 28, 2022

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

@dantevvp
Copy link
Contributor Author

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
Copy link
Collaborator

mvz commented Dec 29, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

mvz commented Jan 6, 2023

@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
Copy link
Owner

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

@dantevvp
Copy link
Contributor Author

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

@bestwebua
Copy link

bestwebua commented Jan 12, 2023

@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: codeclimate/codeclimate#1078, but just silence...

@mvz
Copy link
Collaborator

mvz commented Dec 31, 2023

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

@mvz mvz closed this as completed Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants