lsegal
Refactoring advice before contribution
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:
- Is there an easier/built in way to get kwargs/options from a method call other than what I did?
- Are all the potential
SCOPESoutlined/defined elsewhere? Rubocop was yelling at me to define them in a constant/variable - Is there a way to be able to
evalindividualAstNodesif they're safe? Things like primitives and others... also being able to evaluate instance, class variables, constants etc. in the scope of the currentnamespace
Thanks in advance!
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.
- 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.
- 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.
- 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.
