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

What is the proper way to document a class returned from Data.define? #1533

Open
jcouball opened this issue Feb 9, 2024 · 8 comments
Open
Labels

Comments

@jcouball
Copy link
Contributor

jcouball commented Feb 9, 2024

I am curious how I should document the following:

Milestone = Data.define(:id, :summary, :status)

Such that YARD shows Milestone as a class with the attributes listed above. I can't seem to make it work.

@jcouball
Copy link
Contributor Author

jcouball commented Feb 9, 2024

Below is the best I can come up with.

I don't like how I have to inherit from Data.define. Firstly, it produces a Style/DataInheritance Rubocop violation. The reason this is a Rubocop violation is that an extra class is introduced into the inheritance chain vs. using Milestone = Data.define(...).

YARD also gives the following warning:

[warn]: in YARD::Handlers::Ruby::ClassHandler: Undocumentable superclass (class was added without superclass)

Additionally, to be complete, I would need to define the initializers and other interesting methods provided by the class returned by Data.define.

I am posting this issue to see there there is a better way to document Data objects.

# rubocop:disable Style/DataInheritance

# Defines a project milestone
#
# @api public
#
# @!attribute [r] id
#   The unique milestone identifier (which is a URL)
#   @example
#     milestone.id #=> "https://example.com/db/btkkfcemc?a=dr&rid=197"
#   @return [String]
#
# @!attribute [r] summary
#   A short, one-line milestone summary
#
#   A summary is never blank.
#
#   @example
#     milestone.summary #=> "Doc Storage Requirements Complete"
#   @return [String]
#   @api public
#
# @!attribute [r] description
#   A possibly multi-line / multi-paragraph description of the milestone
#
#   May be blank.
#
#   @example
#     milestone.description #=> "Collaborate with legal on developing doc storage solution"
#   @return [String] the description
#   @api public
#
class Milestone < Data.define(:id, :summary, :description); end

# rubocop:enable Style/DataInheritance

@lsegal
Copy link
Owner

lsegal commented Feb 9, 2024

You have a few options:

The easiest way:

...But technically generates an extra runtime parsed line of Ruby.

Milestone = Data.define(:id, :summary, :status)

# Defines a project milestone
#
# @api public
#
# @!attribute [r] id
#   The unique milestone identifier (which is a URL)
#   @example
#     milestone.id #=> "https://example.com/db/btkkfcemc?a=dr&rid=197"
#   @return [String]
#
# @!attribute [r] summary
#   A short, one-line milestone summary
#
#   A summary is never blank.
#
#   @example
#     milestone.summary #=> "Doc Storage Requirements Complete"
#   @return [String]
#   @api public
#
# @!attribute [r] description
#   A possibly multi-line / multi-paragraph description of the milestone
#
#   May be blank.
#
#   @example
#     milestone.description #=> "Collaborate with legal on developing doc storage solution"
#   @return [String] the description
#   @api public
class Milestone < Data; end
The technically correct Ruby way:

...But uses very awkward YARD syntax.

# @!parse
#   # Defines a project milestone
#   #
#   # @api public
#   #
#   # @!attribute [r] id
#   #   The unique milestone identifier (which is a URL)
#   #   @example
#   #     milestone.id #=> "https://example.com/db/btkkfcemc?a=dr&rid=197"
#   #   @return [String]
#   #
#   # @!attribute [r] summary
#   #   A short, one-line milestone summary
#   #
#   #   A summary is never blank.
#   #
#   #   @example
#   #     milestone.summary #=> "Doc Storage Requirements Complete"
#   #   @return [String]
#   #   @api public
#   #
#   # @!attribute [r] description
#   #   A possibly multi-line / multi-paragraph description of the milestone
#   #
#   #   May be blank.
#   #
#   #   @example
#   #     milestone.description #=> "Collaborate with legal on developing doc storage solution"
#   #   @return [String] the description
#   #   @api public
#   class Milestone < Data; end
Milestone = Data.define(:id, :summary, :status)

@lsegal lsegal added the Question label Feb 9, 2024
@jcouball
Copy link
Contributor Author

jcouball commented Feb 11, 2024

Thanks for the response @lsegal!

I decided to go with your first example. I don't mind the extra line of Ruby code. In the second example, my IDE doesn't format correctly with the "comments nested in comments". So far as I can tell, the end result in the Ruby runtime environment is the same with or without the the extra class Milestone < Data ; end.

One question: in your first example, I don't understand why you can't just replace the last line with:

# @!parse class Milestone < Data; end

I tried that and Milestone was reported as an undocumented object and in the generated documentation, the Milestone class was there but it only knew that it inherited from Data. None of the other class documentation was there.

Thanks for your help!

@topherfangio
Copy link

topherfangio commented Mar 11, 2024

I have a similar situation. I have tried every variation of the above that I can, and none of them appear to remove the warning or actually render the correct documentation.

I kill the server, delete the .yardoc directory, and restart using yard server --reload --bind 0.0.0.0 between each attempt so I don't think it's a caching issue.

I'm using Yard 0.9.34.

Here are the ways I've tried.

First attempts

# @!parse class ModalComponent < LocoMotion::BaseComponent; end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
  # Lots of code here
end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
  # @!parse class ModalComponent < LocoMotion::BaseComponent; end
  # Lots of code here
end

Same thing, but define the language as Ruby

# @!parse ruby class ModalComponent < LocoMotion::BaseComponent; end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
  # Lots of code here
end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
  # @!parse ruby class ModalComponent < LocoMotion::BaseComponent; end
  # Lots of code here
end

Final attempt: "Technically-correct Ruby way"

# @!parse
#   # Create a <dialog>-based Modal component.
#   # class LocoMotion::Actions::ModalComponent < LocoMotion::BaseComponent; end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
# @!parse
#   # Create a <dialog>-based Modal component.
#   # class ModalComponent < LocoMotion::BaseComponent; end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class

Edit:

As a final note, I know that the @!parse is running because if I mess up the code in the comment (like by removing the ; end), it does generate a separate error about not being able to parse that documentation.

@topherfangio
Copy link

Ok, based on this comment, it appears that we cannot suppress the warning, and I suppose that is a separate issue we could open / discuss.

And after playing with it a bit more, I realized I was going about this the wrong way. Here is the code that did what I wanted:

# This is a Modal class!
# @!parse extend LocoMotion::BaseComponent
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
end

This forces Yard to say that the class inherits from BaseComponent rather than Object which is all I was really trying to get it to do 😄

@topherfangio
Copy link

Ok...I'm super confused.

The above code (# @!parse extend LocoMotion::BaseComponent) stopped working and when I re-tried the following it DID work...

# This is a Modal component.
# @!parse class LocoMotion::Actions::ModalComponent < LocoMotion::BaseComponent; end
class LocoMotion::Actions::ModalComponent < LocoMotion.configuration.component_class
end

I guess something was somehow getting cached even though I was deleting the directory every time and re-running the server (maybe something client-side).

The only thing I can think of that changed between my first attempts and this latest / working one is that I declared the Actions module and gave it some basic docs. Previously, I had left it undefined.

So maybe Yard was having trouble finding the base class when one of the modules in the inherited class' hierarchy wasn't defined?

@lsegal
Copy link
Owner

lsegal commented Mar 11, 2024

The extend keyword in Ruby does not define a superclass, it defines a mixin. Mixins are defined in parallel to class hierarcy. YARD would list the mixin as one of the ancestors, but it shouldn't be listing it as as the base class. Indeed following the steps in the comment you referenced is the correct way to do this, namely defining the class, not using extend. More importantly, extend applies to the metaclass, so it's not even really changing the ancestor chain of the class in the way you'd think (it's a bit meta).

IMO the solution here is the same as in the referenced issue, namely, you should just be removing the dynamism and defining the superclass directly:

class LocoMotion::Actions::ModalComponent < LocoMotion::BaseComponent
end

If this is indeed what the superclass is, I don't see the reason to use LocoMotion.configuration.component_class at all. If the constant value can in fact be changed, then your documentation override isn't actually correct in suggesting that BaseComponent is the superclass.

I don't know much about this library, and I'm probably not in a place to recommend API changes here, but if the superclass is expected to change via user configuration, that is a very brittle and poorly formed API structure. If anything, this is the place where extend (more appropriately, include) should be used in order to extend the class with extra functionality rather than dynamically changing the base class based on user configuration:

# make sure ModalComponent always contains _at least_ the features of BaseComponent
class LocoMotion::Actions::ModalComponent < LocoMotion::BaseComponent
  include LocoMotion.configuration.component_class # allow extra component logic to be included
end

Of course, the cleanest way to allow for custom extensions would be to simply allow your downstream consumers to include their own "components" rather than managing it for them in your base classes-- this is exactly why Rails generates ApplicationController type root classes rather than directly inheriting from ActionController::Base; this is what allows users to customize the inheritance chain without allowing them to completely pull the rug out from the inheritance chain. Either that, or use composition over inheritance for these extensions. The tl;dr here should be that if you're allowing users to customize the inherited members, that's fundamentally undocumentable to your library anyway.

@topherfangio
Copy link

@lsegal Yeah, the point of the configuration was allowing downstream users to change the base component class to their ApplicationComponent in case they wanted to extend one of our components and make their own version.

I'm not actually sure we will need this, so maybe it's best to hold off on this (or similar) functionality for later.

Ultimately, though, I did get it working using the @!parse mentioned above and I guess the caching made me think that the extend was what did it.

Thanks for your response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants