Refactoring advice before contribution

[OmriSama]

Hello @lsegal and other maintainers

I've spent the last ~12 straight hours or so writing a Handler for mattr/cattr_accessor in Rails as I was unsatisfied with the code for the ones I could find.

This lead me down a giant rabbit hole, and this is what I've come with:

# frozen_string_literal: true

require 'yard/handlers/ruby/attribute_handler'

module YARD
  module Handlers
    module Rails
      class ClassAttributeHandler < YARD::Handlers::Ruby::AttributeHandler
        handles method_call(:cattr_reader)
        handles method_call(:cattr_writer)
        handles method_call(:cattr_accessor)
        handles method_call(:mattr_reader)
        handles method_call(:mattr_writer)
        handles method_call(:mattr_accessor)
        namespace_only

        SCOPES = [:module, :class, :instance].freeze

        def process
          return if statement.type == :var_ref || statement.type == :vcall

          read = true
          write = false

          method_name = statement.method_name(true)
          case method_name.to_s.split('_').last
          when 'accessor'
            write = true
          when 'reader'
            # change nothing
          when 'writer'
            read = false
            write = true
          end

          # Get standardized parameters
          params = statement.parameters(false).dup

          # Fetch options and defaults
          options = if [:list, :hash].include?(params.last.type)
                      get_options(params.pop)
                    else
                      {}
                    end
          instance_accessor = options.fetch('instance_accessor', true)
          instance_writer = options.fetch('instance_writer', true)
          instance_reader = options.fetch('instance_reader', true)
          default_value = options.fetch('default', 'nil')

          # Define a method for each of the symbols in both scopes
          validated_attribute_names(params).each do |attr_name|
            namespace.cvars << YARD::CodeObjects::ClassVariableObject.new(
              namespace,
              "@@#{attr_name}",
            ) do |cv|
              cv.value = default_value.to_s
            end

            SCOPES.each do |scop|
              namespace.attributes[scop] ||= SymbolHash[
                SCOPES.each_with_object({}) { |k, ob| ob[k] = nil }
              ]
              namespace.attributes[scop][attr_name.to_sym] ||= SymbolHash[
                read: nil,
                write: nil
              ]

              { read: attr_name, write: "#{attr_name}=" }.each do |type, meth|
                if type == :read ? read : write
                  src_meth_name = "self.#{meth}"
                  src_attr_name = "@@#{attr_name}"

                  if scop == :instance
                    next if type == :read && !(instance_reader && instance_accessor)
                    next if type == :write && !(instance_writer && instance_accessor)

                    src_meth_name = meth
                    src_attr_name = "@#{attr_name}"
                  end

                  o = YARD::CodeObjects::MethodObject.new(namespace, attr_name, scop)

                  case type
                  when :write
                    src = "def #{src_meth_name}(value)"
                    full_src = <<~RUBY
                      #{src}
                        #{src_attr_name} = value
                      end
                    RUBY
                  when :read
                    src = "def #{src_meth_name}"
                    full_src = <<~RUBY
                      #{src}
                        #{src_attr_name}
                      end
                    RUBY
                  end

                  o.source ||= full_src
                  o.signature ||= src

                  if o.docstring.blank?(false)
                    o.docstring ||= ''
                    o.docstring += "(Defaults to +#{default_value}+)" unless
                                    default_value.nil?
                  end

                  # Regsiter the object explicitly
                  register(o)
                  namespace.attributes[scop][attr_name][type] = o
                else
                  obj = namespace.children.find do |other|
                    other.name == meth.to_sym && other.scope == scop
                  end

                  # register an existing method as attribute
                  namespace.attributes[scop][attr_name][type] = obj if obj
                end
              end
            end
          end
        end

        def get_options(raw_options)
          safe_types = [
            :array,
            :hash,
            :dyna_symbol,
            :string_literal,
            :symbol_literal,
            :regexp_literal,
            :int,
            :float,
            :kw,
          ].freeze

          eval_if_can = lambda do |node_to_try|
            type = node_to_try.type
            next node_to_try.source unless safe_types.include?(type)

            case type
            when :dyna_symbol, :string_literal, :array, :hash
              next node_to_try.source
            end

            instance_eval(node_to_try.source)
          end

          raw_options.map do |o|
            key = o.first.jump(*safe_types)
            value = o.last.jump(*safe_types)

            [
              (key.type == :label ? key.first : eval_if_can.call(key)).to_s,
              eval_if_can.call(value),
            ]
          end.to_h
        end
      end
    end
  end
end

I want to know if there are any more efficient ways to refactor this, since this was my first foray into the library.

I also want to know:

1. Is there an easier/built in way to get kwargs/options from a method call other than what I did?
2. Are all the potential SCOPES outlined/defined elsewhere? Rubocop was yelling at me to define them in a constant/variable
3. Is there a way to be able to eval individual AstNodes if they're safe? Things like primitives and others... also being able to evaluate instance, class variables, constants etc. in the scope of the current namespace

Thanks in advance!

[lsegal]

Sorry for the late reply.

I want to know if there are any more efficient ways to refactor this

I don't really have the bandwidth to do a full review here, so my initial suggestion would simply be: if it works, it's likely factored well. Golfing code is likely only going to help if driven by tests and specific needs. If you're using a linter that you're happy with, and the linter is happy with it, you should be fine. Generally speaking this is how you would approach the problem.

1. Is there an easier/built in way to get kwargs/options from a method call other than what I did?

Reading from the parameters of the AST is the correct way.

2. Are all the potential SCOPES outlined/defined elsewhere? Rubocop was yelling at me to define them in a constant/variable

I'm not really sure what Rubocop would be asking for here but generally scopes are not defined in any specific place. Note that :module is not a scope in YARD, just %i(instance class) are typically used.

3. Is there a way to be able to eval individual AstNodes if they're safe? Things like primitives and others... also being able to evaluate instance, class variables, constants etc. in the scope of the current namespace

You have .source and on nodes which you can always resolve to code if you truly need to eval. My general recommendation would be to avoid doing this if possible, but it's ultimately a decision you need to make and depends on your risk tolerance level of the library, expected contexts, etc. Generally speaking, rubydoc doesn't whitelist any YARD plugins on the output generation side that make use of eval since they can introduce new attack vectors, but that's just one consideration.

Hope some of this helps.