yard icon indicating copy to clipboard operation
yard copied to clipboard

Default @return on explicit writer method

Open svoop opened this issue 3 years ago • 3 comments

This is best shown with a real life example using a default attribute reader along with an explicit attribute writer which should be documented with overloads as per your comment here:

class Arc
  # Center point
  #
  # @overload center_xy
  #   @return [AIXM::XY]
  # @overload center_xy=(value)
  #   @param value [AIXM::XY]
  attr_reader :center_xy

  def center_xy=(value)
    # do some stuff here
    @center_xy = value
  end
end

However, this documents the writer as follows:

#center_xy=(value) ⇒ Object

While technically correct, it should be:

#center_xy=(value) ⇒ AIXM::XY

It's of course possible to add an explicit @return to the writer docstring.

~~However, shouldn't the default return of the writer better be the return of the corresponding reader – given how Ruby attribute writer methods work these days?~~

Update: Actually, that's not true: Ruby these days always returns the parameter of the writer, so a better default for the writer return would be whatever is set for the @param (in this case AIXM::XY from @param value [AIXM::XY]).

Btw: Thanks for YARD, love the generated docs and how well they work with Dash. 🥳

svoop avatar Apr 06 '22 07:04 svoop

It's of course possible to add an explicit @return to the writer docstring.

Typically you would document @return [void] for an explicit setter. If you couple this with --hide-void-return, you get a realistic representation of your typical attr_* contract.

Even though Ruby's implementation of attr_writer will return the value outright, the actual contract should not be to use attribute setters as return values, i.e., value = obj.center_xy = 1 is a notable code smell that most modern linters will warn you about pretty quickly. This is specifically because your subsequent code that overloads the setter may decide to do fun things after setting the value, and you probably don't want to sign yourself up to follow a contract that shouldn't exist in the first place. Consider the following:

attr_reader :center_xy

def center_xy=(value)
  @center_xy = value
  notify_listeners # best case this returns nil, more likely it will be the array of listener objects
end

A seemingly normal implementation that now breaks the contract. These kinds of regressions can sneak into many codebases, and I almost guarantee you don't have any unit tests ensuring the return values of your attr_writers.

That said, if you're truly set on enforcing that your setters return the object back to the caller, you can absolutely document this with YARD in the following way:

class Arc
  # Center point
  #
  # @overload center_xy
  #   docs for reader
  # @overload center_xy=(value)
  #   docs for writer
  #   @param value [AIXM::XY] param docs
  # @return [AIXM::XY]
  attr_reader :center_xy
end

Note the @return tag is outside of the overloads. This gets properly applied on the overload signatures and renders the following in the default template, which is probably more streamlined for this specific use case anyway:

image

But you should really @return [void] IMO.

Hope that helps!

lsegal avatar Jun 10 '22 18:06 lsegal

attr_reader :center_xy

def center_xy=(value)
  @center_xy = value
  notify_listeners # best case this returns nil, more likely it will be the array of listener objects
end

Note that this comment is incorrect. As pointed out in the original issue, Ruby always returns the parameter as the result of an assignment expression. No amount of funny business in an assignment method can change its return value.

ccutrer avatar Sep 13 '22 17:09 ccutrer

Ruby always returns the parameter as the result of an assignment expression

"assignment expression" is the key term here. The (lexical) assignment expression syntax is the reason Ruby returns the right-hand-side value, not the method (this is true for any assignment syntax, not just attributes), but the method still returns the wrong value-- the method is just obscured by a specific Ruby syntax. This is actually even more misleading when you consider this syntax-vs-method gotcha:

class A
  attr_reader :value
  def value=(v) :NEVER_ASSIGNED_A_VALUE end
end

a = A.new

puts(a.value = 1) # Assignment syntax is what returns 1 here
puts(a.send(:value=, 1)) # The actual method return value

# Finally, inspect the attribute only to find that Ruby never set it
# in the first place. The first assignment return value was a lie :)
p(a.value)

Playground link

Remember, YARD documents semantic behavior of your API, not Ruby syntax. It's actually fairly important to distinguish between the convenience of Ruby syntax and what your API actually does for the very reason above.

lsegal avatar Sep 13 '22 18:09 lsegal