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

:ditto: doesn't work with :inherit: #11362

Open
Blacksmoke16 opened this issue Oct 25, 2021 · 5 comments
Open

:ditto: doesn't work with :inherit: #11362

Blacksmoke16 opened this issue Oct 25, 2021 · 5 comments

Comments

@Blacksmoke16
Copy link
Member

Given the following class setup:

abstract class Parent
  # Runs the thing.
  abstract def run : Nil
end

class Child < Parent
  # :inherit:
  def run : Nil; end

  # :ditto:
  def run(some_bool  : Bool) : Nil; end
end

Produces:

image

I would have expected the :ditto: to run after copying the docs from the parent method.

@Blacksmoke16 Blacksmoke16 added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Oct 25, 2021
@straight-shoota
Copy link
Member

What about this case:

class Parent
  # foo
  def foo; end

  # bar
  def bar; end
end

class Child < Parent
  # :inherit:
  def foo; end

  # :ditto:
  def bar; end
end

You could reasonably expect the documentation for Child#bar be bar, i.e. :inherit: expandeds after :ditto:, not before. That's how it currently works.

@straight-shoota straight-shoota added status:discussion and removed kind:bug A bug in the code. Does not apply to documentation, specs, etc. labels Oct 25, 2021
@Blacksmoke16
Copy link
Member Author

It probably works that way because :inherit: was added after :ditto: and this scenario wasn't considered back then. While technically correct there is no reason to not use :inherit: instead of :ditto: in this context.

Another thing to consider is how they should work if you did like:

class Child < Parent
  # :inherit:
  def foo; end

  # :ditto:
  # 
  # :inherit:
  def bar; end
end

I would think it should output:

foo

bar

But currently you get:

image

@straight-shoota
Copy link
Member

straight-shoota commented Oct 25, 2021

The technical reason is probably that :ditto: is resolved statically. It's in the top level visitor. Only :inherit: is specific to the doc generator. That just happens to be later in the process.

I was wondering why in your last example :ditto: does not get expanded, but that's #11332 (and thus unrelated). Anyway: If that worked, yeah it would probably make more sense to result in foo bar than bar bar. But really, what's the use case for this? I can't help but think this is mostly a theoretical discussion.

When considering concrete use cases, I could think of overriding methods with some additional text that amends the parent docs and is shared among several methods.

class Parent
  # This does foo.
  def foo; end

  # This does bar.
  def bar; end
end

class Child < Parent
  # :inherit:
  #
  # And it's done by a child.
  def foo; end

  # :ditto:
  def bar; end
end

In this case again, I would clearly expect the doc for Child#bar to be "This does bar. And it's done by a child."

@Blacksmoke16
Copy link
Member Author

But really, what's the use case for this? I can't help but think this is mostly a theoretical discussion.

I have a module like:

module Athena::Console::Style::Interface
  # Helper method for asking `ACON::Question` questions.
  abstract def ask(question : String, default : _)
end

Then an implementation of the module (cut down for this example):

struct Athena::Console::Style::Athena
  include Athena::Console::Style::Interface

  # :inherit:
  def ask(question : String, default : _)
    self.ask ACON::Question.new question, default
  end

  # :ditto:
  def ask(question : ACON::Question::Base)
    # ...
  end
end

It made sense to me to want to copy the docs from the interface implementation on the other overload, and can't just use :inherit: their since its signature isn't on the parent. Of course could just manually add the docs but 🤷, then it's easier for things to get out of sync.

@HertzDevil
Copy link
Contributor

A use case in #11368 is as follows. Currently we have:

class Array(T)
  include Indexable::Mutable(T)

  # The quick brown fox jumps over a lazy dog.
  def fill(value : T, start : Int, count : Int) : self
    # ...
  end

  # :ditto:
  @[Deprecated("Use `#fill(value, start, count)` instead")]
  def fill(value : T, *, from start : Int, count : Int) : self
    fill(value, start, count)
  end
end

That PR makes the first overload available in Indexable::Mutable, so it makes sense to move the documentation there. But Array overrides it nonetheless because of a Slice optimization. Thus we now have:

module Indexable::Mutable(T)
  # The quick brown fox jumps over a lazy dog.
  def fill(value : T, start : Int, count : Int) : self
    # ...
  end
end

class Array(T)
  # :inherit:
  def fill(value : T, start : Int, count : Int) : self
    to_unsafe_slice.fill(value, start, count)
    self
  end

  # :ditto:
  @[Deprecated("Use `#fill(value, start, count)` instead")]
  def fill(value : T, *, from start : Int, count : Int) : self
    fill(value, start, count)
  end
end

Now the deprecated overload's documentation is incorrect, and the original doc must be copied to one of Array's overloads.

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