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

Automatically inherit docs from previous def and/or ancestors when missing #6989

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@asterite
Copy link
Member

commented Oct 25, 2018

NOTE: Final behavior is in #6989 (comment)

This is something that should have been in the doc generator from the beginning.

This PR makes it so that if you don't document a method and that method either redefines a previous method (overwrites it) or is an override (it overrides the method of a superclass), the docs of that other method will be used.

For example:

class MyClass
  # Some docs
  def foo
  end
end

class MyClass
  def foo
  end
end

In the above case we redefine a method, and the doc generator will use the original doc comments because we are not specifying any.

The other case is:

class MyClass
  # Some docs
  def foo
  end
end

class MySubclass < MyClass
  def foo
  end
end

Here we are overriding a method in a subclass, and because we are not specifying any docs the doc generator will use the ones from the superclass.

I think it always makes sense to "inherit" the docs when we don't specify them because the current behavior is not showing anything at all, and that isn't very useful. We either want to inherit the docs (after all, a subclass method should behave more or less the same way as a superclass method) or we want to specify other docs.

Then one could think of having :inehrit: or something like that to inherit docs and add more docs. Let's please discuss that in a separate issue/PR.

My other PR about the @[Redefine] and @[Override] annotations are slightly related but not quite: the annotations won't be used by the doc generator, they are just for the compiler to check that your intentions match the behaviour of the compiler. The compiler already knows when you redefine or override a method. So these PRs are orthogonal.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

I think it always makes sense to "inherit" the docs when we don't specify them because the current behavior is not showing anything at all, and that isn't very useful.

I'd also not consider it very useful to just display some documentation of a previous/superclass definition, whose behaviour has most likely changed in some way.

If documentation is implicitly inherited, it can be highly misleading when it plainly states incorrect facts. If there is no documentation at all, it is at least evident that this is missing.

Javadoc implicitly inherits documentation, but adds a reference to the origin: Description copied from ParentClass.
There is also an {@inheritDoc} directive which copies the documentation without the reference.

An overview of the different behaviour can be found in this article: https://christianmoser.me/sinn-unsinn-von-inheritdoc/ The article body is in German but the examples are self-explanatory.

It's also a nice feature to point to the overriden method (regardless of documentation text), but I guess that's for a different PR.

@asterite asterite force-pushed the asterite:feature/copy-docs branch from e5892f6 to f28d634 Oct 25, 2018
@asterite

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

@straight-shoota I like the Description copied from ... thing.

What do you think if we do this:

  • If no documentation is provided, we copy it from the ancestor, if available, saying Description copied from #{ancestor}
  • For the case of copying from a previous_def... I'm not sure whether it makes sense to say Description copied from a previous def, the reader has no idea what's going on, so I think copying it is fine.
  • If you write :inherit: as a doc comment, it will inherit the docs without the Description copied from ... thing
  • If the docs start with :inherit: or end with :inherit: (in a single line) we can copy the parent docs plus add the extra doc that's written there.

Once we agree, I'll also write tests for this.

@Sija

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

Most clear for me would be explicit :inherit: comment (only at the beginning) with optional suffix which gets appended (separated by an empty line).

@asterite

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

Yes, you (community) decide what you think is the best approach. Once you decide that, let me know and I'll implement it.

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

👏 👏 👏 simple solution & tiny patch!

I don't see any reason to not inherit the doc from a redefined method by default.

Maybe an explicit :inherit: would be nice for looking up the ancestors? That being said, I don't think documenting the existence of the override without an specific doc has much value. It could be :nodoc: if it adds nothing particular (i.e. implementation detail) or it should have an explicit doc, possibly referring to the ancestor doc if needed.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

💯 This is going to be great. I have a abstract parent class that 4/5 classes inherit from that all share some characteristics. Being able to define the docs on the parent class and have it appear on all the child classes, plus the child specific info would be super slick; instead of keeping the same doc block on each subclass.

@RX14

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

How about injecting the "Description copied from..." notice inside the HTML doc generator, and display it in a different style to make it distinct from the rest of the doc comment (but less visible, not more like NOTE:). It'd probably be nice to be able to distinguish in the json output whether the doc comment was inherited or not.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@asterite I think this would be a great addition, is it still on your radar? =)

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@straight-shoota Yes, but I'm not sure which direction we should take.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I'd go with #6989 (comment). Would be nice to have something and can fine tune it as needed.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@asterite Any plans on wrapping this up? #6989 (comment) has quite a few 👍 and seems to be a clear winner.

Would be nice to have something useable and can iterate on it in the future if needed.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@Blacksmoke16 I'll do it soon, maybe tomorrow I'll update this PR.

@asterite asterite force-pushed the asterite:feature/copy-docs branch from 4b5448a to 975b3d9 Aug 16, 2019
@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

Updated with the behavior described in #6989 (comment)

For a code like this:

class Foo
  # Some docs for foo
  def foo
  end

  # Some docs for bar
  def bar
  end

  # Some docs for baz
  def baz
  end

  # :nodoc:
  # I can still document it!
  def hidden
  end
end

class Bar < Foo
  def foo
    super
  end

  # :inherit:
  def bar
  end

  # The old method did this:
  #
  # ---
  #
  # :inherit:
  #
  # ---
  #
  # But this one just rocks!
  def baz
  end
end

The docs will look like this.

Instance method summary (what you see at the very top):

Screen Shot 2019-08-16 at 6 09 58 PM

Instance method detail (what you see below the summary):

Screen Shot 2019-08-16 at 6 10 04 PM

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

@asterite This doesn't seem to work as expected when inheriting from non-methods. I.e. a class/struct or macros.

# Some common docs for Foo
abstract class Foo
  # A macro
  macro mac
  end
end

# :inherit:
#
# Some specific docs for Bar
class Bar < Foo
  # :inherit:
  #
  # Other
  macro mac
  end
end

image

image

Other than that, this is awesome!

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

Class docs are never inherited and I think it's wrong to do so. I don't believe any language does that.

For macros we don't track overriding, because for macros you must call them like Foo.macro and Bar.macro so there isn't really overriding... well, unless you call them from an instance method but I feel that's way less common. And anyway we don't track overriding (as in, there's no code to figure out whether a macro overrides another macro) so I won't do that in this PR. Maybe something for the future.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Works for me. Was just something I noticed, don't really have a use case for any of it.

I'm assuming the same is true for initializers/class methods?

Copy link
Member

left a comment

Please update de opening message with the link to the final behavior.

This make me think about how inherited methods work with deprecations. Nothing to do with the PR itself, but something to keep in the radar

@RX14
RX14 approved these changes Sep 3, 2019
@asterite asterite added this to the 0.31.0 milestone Sep 3, 2019
@asterite asterite merged commit 9fb139a into crystal-lang:master Sep 3, 2019
5 checks passed
5 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite asterite deleted the asterite:feature/copy-docs branch Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.