Add helper to handle method decorators

[amclain]

Expanding on the work of PR #767 for issue #760, I've added the ability for private_class_method to be handled correctly when used as a method definition decorator, which is valid syntax as of Ruby 2.1.0.

[lsegal]

If we're going to add this, I'd like to see it added consistently across all visibility methods. That said, is this actually a common idiom?

[amclain]

Method definition decorators were a major feature made possible in Ruby 2.1.0, so in general yes, that part is a common idiom (a project I’m a developer for makes significant use of this feature). Specifically pertaining to private_class_method though, no, it would be uncommon to use it this way and most of the time probably isn’t best practice. I came across the decorator issue when playing around with some code and Yard raised an exception; this happened to be a piece that I was familiar enough with to fix.

Based on what I learned creating this patch, there are two distinct tasks that the handler for private_class_method as a method decorator has to accomplish:

1. Register the method definition and transfer any docstring from the statement to the method.
2. Set the visibility of the method.

Ideally the private_class_method handler should only be dealing with the part where visibility is set. The problem is that Yard’s current behavior seems to ignore method definitions that come after decorators. This was originally causing the private_class_method handler to raise an exception when used as a decorator because the method it was trying to set visibility on wasn’t getting registered. In the broader case of a developer creating their own decorator, Yard seems to ignore the method definition, as illustrated in the following code:

class DecoratorExample

  # Add functionality to a method.
  def self.my_decorator method
    imeth = instance_method method
    define_method method do |*args, &block|
      "does stuff to " +
        imeth.bind(self).call(*args, &block)
    end
  end

  # Returns a description of itself.
  my_decorator def foo
    'the foo method'
  end

end

DecoratorExample.new.foo
# => “does stuff to the foo method”

In this case class method my_decorator is documented, but instance method foo is not.

So yes, I would agree with you that all the visibility methods that can behave like this should be updated. However, without a mechanism for handling decorators in general, I could see a fair amount of code duplication happening, and I think part of the problem would be getting solved in the wrong place.

What’s the best way to proceed at this point? Should I open a ticket for discussion on implementing a way to handle decorators in general?

[lsegal]

YARD has a generalized mechanism for handling "decorators" through handlers / plugins. In other words, it should be done the same way we are doing it now. Duplication can be dealt with by putting common behavior in mixins that are shared across handlers.

YARD should be supporting the builtin methods out of the box, but not generically handling all "decorators". That's the job of third-party plugins to provide handlers for custom DSL code. Basically, what we need to do is update YARD to handle this new "methods can be expressions passed to visibility declarations" syntax, and let third-party plugins handle the cases that are not part of the core language.

[amclain]

I did some more research and experimentation, and I think we’re on the same page.

I was using the term “decorator” a little too liberally before; the problem I saw was specifically with decorators in front of a method definition. I thought there was a more general problem caused by the method definition being shadowed in the AST by the decorator, but I may be wrong about this. I can bring that problem back up later if it does turn out to be an issue.

Regarding handling the built-in visibility methods, yes, I think that makes sense. Regarding custom decorators being handled through the existing mechanisms by third-party plugins, yes, I agree.

The implementation of "methods can be expressions passed to visibility declarations" (or passed to any method definition decorator in general?), sounds like it may fall into a whole different scope of work that involves some design decisions. Is this something I should actually try to do myself, or should I wait for you or another major maintainer to implement the functionality, then piggyback the visibility methods off that?

[amclain]

I looked at this from a different perspective and started working on an implementation. It's easier than I thought.

[amclain]

I've created a DecoratorHandlerMethods mixin at your recommendation and it seems to have solved a lot of the problems. YARD's built-in visibility handlers that can decorate method definitions have been refactored using this mixin. DSL handlers are completely left up to third-parties to implement, with an opt-in to include the mixin so they don't have to roll their own support for decorator chaining, method defintion parsing, and such.

This PR should be ready to go if you approve.

[baburdick]

Any plans to merge this?

[amclain]

I can update this branch if there is acceptance to it being merged in.

[lsegal]

I realize that the new composable "syntax" (driven by methods returning symbols) is something that a lot of developers want to begin using with YARD. I have no objections to merging this in theory, but there is quite a lot of churn in the commits that make it hard to follow. Specifically I want to ensure that:

1. No existing functionality is being changed (this should be obvious from the tests passing)
2. No existing APIs are being changed in a backwards incompatible way
3. Most importantly, the functionality being added here should not be specific to private_class_method and should work with any of the existing DSL methods supported by YARD (private, public, etc). Ideally we'd want this to work for any DSL method, but that might be something that comes later.

If you can confirm these things I'm okay with taking a deeper dive on this and getting it in. It would also help to re-summarize what this PR is doing so we're all clear what the functionality is (re: point 3). There was some confusion mid-way through because the original patch was for something very different, and the original PR title / description is (or should be) obsolete.

[amclain]

It's been so long that I need to review the PR as well, but I believe that all three points are ok. I can also try to squash some of the commits while I'm at it.

[amclain]

@lsegal I've fixed the merge conflicts and squashed the commits.

---

Summary:

This PR adds the opt-in module YARD::Handlers::Ruby::DecoratorHandlerMethods to assist in processing Ruby method decorators. It is used internally for the public/private visibility handlers, and can also be easily included into third-party handlers.

For example, given the decorator foo:

class Test
  # The foo decorator.
  def self.foo(method)
    "foo " + new.send(method)
  end

  # The bar method.
  # @return [String] bar modified by {foo}
  foo def bar
    "bar"
  end
end

A handler can be created:

class Handler < YARD::Handlers::Ruby::Base
  include YARD::Handlers::Ruby::DecoratorHandlerMethods

  handles method_call(:foo)

  def process
    process_decorator
  end
end

And the docstring is passed through to #bar:

process_decorator can also accept a block if #bar's docs need to be modified based on how the decorator modifies it:

process do
  # Set visibility of a class method to private.
  process_decorator :scope => :class do |method|
    method.visibility = :private if method.respond_to? :visibility=
  end
end

[baburdick]

Would it make more sense here to use .respond_to?(:[]) instead of .is_a?(Hash)? What if the implementation changes to use a Hash-like object not descended from Hash?

[lsegal]

That seems unlikely and would break the API in all sorts of other ways. The is-a check is sufficient, and [] may not be the only thing our implementation expects from the node argument, so validating that it's a Hash seems correct to me.

[baburdick]

Fair enough.

[lsegal]

I finally had time to look through this PR (sorry for the delay!). In general I think it's good. I threw in some brief comments and questions inline for most of these, but I have some generalized comments:

The process_decorators method seems a little heavy handed in a few places. Specifically, I don't really understand the value of transfer_source and transfer_docstring. Given that users have a block form callback, they can reset any data they want if they don't want to copy over source or docstrings (i.e., revert), though these seems extremely uncommon to want to do.

More importantly though, I'd like to understand what transferring the docstring actually does. I found this to be the hardest part of the code to understand. I see that you are copying over the unparsed docstring data from the statement object, which I understand to be necessary (since the underlying method declaration will not have picked up the docstring data), but you then copy over the tags from the method object, which I would have suspected to be empty (since, again, the docstring data would not be associated with the method). Seeing the already-complex conditional include method.docstring.empty? concerns me too, because it makes me think something else is at play here.

Those were my two main issues with this. With the outstanding comments and questions resolved, I think it's getting close to being merged. Thanks for working through all of this, @amclain!

[amclain]

The comments disappeared after I pushed some changes. Here's the link to the changeset with them:

amclain@534e132#commitcomment-15629753

[lsegal]

For context I'm going to add follow ups to the original commit, please follow along there.

[lsegal]

Added extra comments, please follow up on any of the comment threads that have not yet been responded to. Thanks!

Another high-level design point I forgot to bring up:

How does your chaining algorithm handle evaluation order of decorators? Specifically, the evaluation order with which decorators are used is important to get right. To be correct, your helper would have to handle decorators from right to left, not left to right, since Ruby evaluation would happen in RTL.

Consider two decorators, cpub and cpriv. cpub sets visibility to public, cpriv sets visibility to private (similar to public and private but chainable):

cpub cpriv def foo; end

The above code should actually cause #foo to be public, because cpub actually gets evaluated last.

For YARD to be correct, it would have to emulate this order of evaluation. In my glance at the test suite, I only see one indirect test for order and it looks like it's validating LTR behavior.

To summarize: the behavior of chaining should evaluate right-to-left, and we should add explicit test cases for this scenario.

[lsegal]

This took a while, but it's in. Thanks for the hard work on this, @amclain!

[amclain]

Awesome, thanks @lsegal!