Issue #5759 -- inherit documentation

[dscottboggs]

This issue came to my attention and I thought I'd take a stab at it. I hope this solution works, and is the way you want to go about resolving this issue.

[Sija]

just :inherit:, ditto all below (btw could it get more DRY?)

[Sija]

also I'd change it to str == ":inherit:"

[bew]

what if you want to inherit docs and add some specific doc afterward?
like:

# :inherit:
# Some specific doc
def foo
end

[Sija]

@bew I thought about but it's IMO better to add it in another PR since it'd be beneficial for :ditto: too

[straight-shoota]

what if you want to inherit docs and add some specific doc afterward?

I'm not sure that makes sense with :inherit: or :ditto:. What would it mean? Just append everything following the keyword? Or ignore it as internal comment?

[bew]

I'd say append everything, for internal comments you'd use :nodoc: or put the doc inside the method (as a workaround)

[asterite]

This should use @type.ancestors which gives you the full type hierarchy (including modules) because @type.superclass just gives you the superclass.

[asterite]

This is also wrong because it just checks for the name, ignoring argument names and types.

[asterite]

This is good!

Though I'm thinking it's probably better to have an @[Override] annotation which will error if a method doesn't override another one. The problem is defining the semantic of this, but for starters we can require having the exact same signature. I'll start to work on this. And once we have this we can replace :inherit: with this.

[RX14]

@asterite aren't they orthogonal?

[asterite]

@RX14 If you override a method and you don't specify docs, I think it makes sense to inherit the docs.

[straight-shoota]

@asterite https://github.com/GrottoPress/crystal-annotation has a simple @[Override] annotation.

And once we have this we can replace :inherit: with this.

Wouldn't it make more sense to don't even merge :inherit: in the first place, even if it means waiting a little bit for @[Override]?

But in general, I like this either way.

[RX14]

@asterite inherit allows composing documentation by adding :inherit: and a suffix.

[Sija]

@RX14 not with the current implementation from this PR, but as I said earlier in #6981 (comment) it's sensible to do it in a followup PR.

[asterite]

@RX14 Ah, that makes sense. Well, in any case, the implementation in this PR is wrong. One should copy the logic of my "Redefine and Override annotation" PR.

[straight-shoota]

inherit allows composing documentation by adding :inherit: and a suffix.

I don't think that's actually much helpful, it might rather be contraproductive at times.

When a method is overriden in a child class, it usually changes behaviour from the parent implementation. This could either be only internal changes, not worth mentioning. Then just inheriting the parent's docs is enough. The can be achieved with either :inherit: or @[Override].
If the changes are relevant to the public API, this should be mentioned in the main part of the method doc, typically near the top.
The option to just append something to the parent's docs, is probably not much help for most use cases.

[ysbaddaden]

My use case for :inherit:, or an annotation that would copy the documentation, is Crystal::System.

For now, we have documented methods that often delegate to another type/module under the Crystal::System namespace, because otherwise we'd replace the method and lose the documentation (or have to duplicate it). It would be great to have documented abstract methods on types and the implementations in target specific files by reopening the types, yet keep the documentation from the abstract methods.

I'm fine with always copying the docs from an abstract method when it's implemented, or make it explicit with :inherit:, or introduce an annotation for stricter checks, ...

[straight-shoota]

@ysbaddaden In your Crystal::System implementation example, the methods wouldn't be inherited, so abstract, :inherit: or @[Override] wouldn't apply anyway.

And honestly I can't see any benefit over documenting a public API method which calls a private implementation method defined in Crystal::System.

[dscottboggs]

Sorry about jumping the gun on this PR, I knew like right after uploading that my implementation wasn't going to cut it. I'll do more thorough testing and research next time.

I was going to begin working on this now, but I saw @asterite 's #6989 and I like that syntax way better than the one suggested by @bew that inspired this PR.

[RX14]

@dscottboggs thanks a lot for making this PR! The compiler is a very complex piece of code.

[asterite]

@dscottboggs Also it's thanks to you I decided to tackle these issues myself :-)