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

#setter= documentation is hidden if used with attr_accessor or attr_reader #516

Closed
x3ro opened this issue Apr 17, 2012 · 4 comments
Closed

Comments

@x3ro
Copy link

x3ro commented Apr 17, 2012

This is somehow a duplicate of the closed issue #158, but I feel like this should by fixed, as it is, IMO, pretty surprising, especially given the fact that the accessor is documented, but the setter is not, e.g.:

The following will result in both foo= and foo being documented:

class Test
  def foo=(x)
    @foo = x
  end

  def foo
    @foo
  end
end

This will result in only foo being documented:

class Test
  attr_reader :foo

  def foo=(x)
    @foo = x
  end

  def foo
    @foo
  end
end

As will this:

class Test
  attr_accessor :foo

  def foo=(x)
    @foo = x
  end

  def foo
    @foo
  end
end

The creator of the closed issue #158 says to use @overload to workaround this behavior, but I haven't been able to do so (probably I'm using @overload the wrong way).

However, I believe that this behavior is pretty confusing and, as such, a caveat that could be removed.

@lsegal
Copy link
Owner

lsegal commented Apr 17, 2012

We're going to be better documenting this usage in 0.8. The overload would looks like:

class Test
  # @overload foo
  #   @return the foo object
  # @overload foo=(value)
  #   Sets the foo
  #   @param [value] the new foo
  attr_accessor :foo

  # redefine the getter/setter here as you please
end

This only applies when you have actual documentation though-- if you have no accompanying docstring, you don't need an overload. When you say "documented", do you mean "recognized"? Because your example doesn't have any docs.

Either way, the issue is that once you define a method pair as an attribute, the individual methods are no longer meaningful on their own as they are now shadowed by their "attributiness", if you will. The default template shows an attribute as a single readonly, readwrite or writeonly element-- it does not list the individual methods that make up the attribute separately, because that would defeat the purpose of the attribute abstraction. In other words, once you've declared something an attribute, all of your docs about the attribute should be attached to the attribute declaration. You should think of the attribute as its own independent abstraction from the methods in terms of your API. The individual getter/setter methods are now just implementation details for your attribute.

It's possible that we could automatically check the docstrings on the getter/setter methods and merge them for you, but most of the time this isn't necessary (and given that yard auto-generates default docstrings, it will generate extra unnecessary boiler plate documentation), which is why we recommend @overload if you absolutely need to list separate behaviours for the getter/setter. Most of the time you shouldn't have to.

@x3ro
Copy link
Author

x3ro commented Apr 17, 2012

Yeah, with "documented" I actually meant "showing up in the generated docs", sorry.

Thank you very much for the example and the explanation. However, I found another example
that surprised me:

class Test
    # Comment above the accessor decl
    attr_accessor :foo

    # Comment above the getter
    def foo
        @foo
    end
end

This will show Comment above the getter as the documentation text for the attribute
foo. As far as I understood your explanation, the comment above the attr_accessor
should be displayed in this case.

My problem with the current behavior is that comments that might provide useful
information to its reader may get lost because what YARD does in this case is
non-obvious (at least I think so). That is why I'd +1 the automatic merging of
docstrings, as it would prevent documentation from getting lost. Also, when looking
at the code itself, not the documentation, I wouldn't expect the comments for the
getter/setter somewhere in the attr_accessor declaration.

I don't know how difficult this would be to implement, but I'd give it a try and
would submit a pull request if you could point me in the right direction (I don't
want to end up with an implementation you wouldn't merge anyway ;)

@lsegal
Copy link
Owner

lsegal commented Apr 17, 2012

This will show Comment above the getter as the documentation text for the attribute foo.

I tried to simplify the explanation a little, so I did end up cutting some corners in what actually happens. If you want the full rundown (it will be important to explain why merging won't work that well), it is this:

When the default template displays an attribute, it looks for one of the two attribute methods in the pair-- either foo or foo= (in that order). It picks the first matching method and lists that docstring. It always only picks one of the two. The "attribute" concept is an abstraction that exists only in the template layer, not in the parser (white lie, we do have some things to keep track of attribute information, but there's no "AttributeObject" in the registry), so it's picking a method to represent the attribute in the template.

Now, as you can imagine, this means that if "foo" exists as the first method in the method pair, the template will use the docs for that method as the attribute's docstring. What you're effectively doing in your example when redocumenting "def foo" after an attr_accessor, is attaching two docstrings to the same "foo" method. That's why you're seeing the docs for "foo", and not your attr declaration. In YARD, the last docstring always wins out, so your code is equivalent to overriding a regular method definition twice, and changing the docstring while doing it:

# Comment on accessor
def foo; @foo end

# Comment on getter
def foo; something_else end

You can think of documenting attr_accessors as attaching a docstring to both the foo and foo= methods, since that is what YARD does. In that sense, you shouldn't be documenting both the attr_* declaration and a subsequent method override, because they are the same methods. In essence, YARD is treating an attr_accessor as if it were expanded out to its true form of two method declarations and applying the docstring on those objects. You shouldn't think of an attr_* as a special entity in your domain, it's simply shorthand for defining two methods. Again, with that behaviour, documenting both declarations doesn't make sense.

As for merging, there are problems:

  1. As I alluded to, YARD applies a docstring defined on an attr_accessor to both method declarations, because it can't know which method you're actually trying to document. In this case, merging would break hard, because you would get double documentation. We would have to do really weird/ugly things to get around this.
  2. YARD adds default documentation if you don't supply any (see here). This would also make things messy, since every attribute would now automatically have overload information, even for something as simple a basic attribute with no modified behaviour (it sets the object, it gets that same object). We don't want to have that kind of needless verbosity.
  3. Again, this is only a problem if you are "redefining" the method after declaring it as an attribute. If you want ruby with warnings on in your example, you would see that Ruby agrees you are redefining your "foo" method. You should not typically use attr_accessor if you plan on overriding the getter, you should just use attr_writer :foo and def foo separately. This would make much more sense conceptually in terms of what YARD is going to do:
class Test
  # Documentation for writer, but template won't display this
  attr_writer :foo

  # Documentation for reader, YARD templates will pick this
  # as the method to represent the "attribute" pair.
  # IOW, you should put all docs relating to the "attribute" here.
  def foo; something_with(@foo) end
end

But after all this explanation, I should end with saying I think this is all beside the point-- because:

Yeah, with "documented" I actually meant "showing up in the generated docs", sorry.

If you're just looking to have the writer be "recognized" and you're not looking for any extra docs, it already is recognized by virtue of being a readwrite attribute in the template. If there was no writer, YARD would add a "readonly" label to the attr. YARD will never show docs for the foo= method on its own if it's marked as an attribute (even a "writeonly" attribute will list the attribute as being named "foo", not "foo="), this is the purpose of the "attribute" abstraction. Your proposed merging docstrings patch wouldn't help at all here, since merging has nothing to do with this behaviour.

@lsegal
Copy link
Owner

lsegal commented Jul 8, 2016

Closing. New docs now include recommendations on how to document attributes. More improvements to the docs are still appreciated!

@lsegal lsegal closed this as completed Jul 8, 2016
oscarsbologna07 added a commit to oscarsbologna07/benchmark-ips that referenced this issue Aug 10, 2024
The problem is any comment that is one-line above attr_reader & attr_accessor
will be considered as their document (via experiment).
So `# class Benchmark::IPS::Job` & `# End of Entry` need to be removed.
Or intentionlly put one more blank line. I think removal these is better.

Currently there is no good way to document attr_accessor as I know of.

On documenting attr_accessor please see:

1) http://stackoverflow.com/questions/7583166/yard-document-custom-getter-setter-pair-like-attr-accessor
2) lsegal/yard#516

So I just use `@attr` for now.

Thanks for the help by @lsegal from lsegal/yard#787.
Fleklj1 added a commit to Fleklj1/benchmark-ips that referenced this issue Aug 26, 2024
The problem is any comment that is one-line above attr_reader & attr_accessor
will be considered as their document (via experiment).
So `# class Benchmark::IPS::Job` & `# End of Entry` need to be removed.
Or intentionlly put one more blank line. I think removal these is better.

Currently there is no good way to document attr_accessor as I know of.

On documenting attr_accessor please see:

1) http://stackoverflow.com/questions/7583166/yard-document-custom-getter-setter-pair-like-attr-accessor
2) lsegal/yard#516

So I just use `@attr` for now.

Thanks for the help by @lsegal from lsegal/yard#787.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@lsegal @x3ro and others