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

Filtering included docstrings #839

Open
thofma opened this issue Sep 22, 2018 · 5 comments
Open

Filtering included docstrings #839

thofma opened this issue Sep 22, 2018 · 5 comments

Comments

@thofma
Copy link
Contributor

thofma commented Sep 22, 2018

What are the rules for filtering docstrings? Consider the following function

julia> struct A{T} <: Number end
julia> """Number""" foo(x::Number) = x; # 1
julia> """Real""" foo(x::Real) = x; # 2
julia> """A{T}""" foo(x::A{T}) where {T} = x; # 3
julia> """A{Int}""" foo(x::A{Int}) = x; # 4

It is clear that @docs foo(::Real) and @docs foo(::Number) should return #1 and #2 respectively (and they do). But what about the following?

  1. Should @docs foo(::A{T} where {T}) give #3?
  2. Should @docs foo(::A{Int}) give #4?
  3. What should @docs foo(::Int) do?
  4. What should @docs foo(::A{Float64}) do?

Now here is what is currently happening:

  1. Pulls in #1 and #3.
  2. Pulls in #4.
  3. Pulls in #1, #2, #3.
  4. Pulls in #1 and #3.

P.S.: You can check the above yourself by doing Documenter.DocSystem.getdocs(Documenter.DocSystem.binding(Main, :foo), Tuple{A{T} where {T}}).

@mortenpi
Copy link
Member

I am not sure what the intended logic currently is, but I think I would expect it to get all the docstrings that are more specific than the signature specified. So IMO

  1. foo(::A{T} where {T}) should give 3 and 4.
  2. foo(::A{Int}) should give 4
  3. foo(::Int) shouldn't match anything.
  4. foo(::A{Float64}) shouldn't match anything either.

Additionally:

  • foo(x::Number) should give all four.
  • foo(x::Real) should give 2.

I also initially thought that it should be consistent with the REPL help?> prompt, but I am no longer sure. In the REPL, if you e.g. help?> foo(::Float64), then you should probably see all the general ones that match, especially since there might not be a specific method even defined (matches "upwards", if you will).

Whereas in Documenter I think we can assume that the doc author knows exactly which methods they want to document, so it should match "downwards". And if the author specifies the signature, they probably want to filter out unrelated more general docstrings.

Thoughts on this would be very welcome though.

@thofma
Copy link
Contributor Author

thofma commented Sep 25, 2018

Thanks for the feedback. Note that with JuliaLang/julia#29102 the REPL behavior will change. In particular help?> foo(::Float64) will give you only the docstring for the method which will be applied. Once the PR over at julia lands, only the docstrings of methods which are applicable for a given signature will be shown (so matches are "downwards").

But I agree that Documenter should not do the same as the REPL. I thought some time about this issues and at the beginning I came up with the same rules as you. But I think foo(::Number) printing all four has the serious drawback that you can never include docstring #1 alone at one pace. I feel that this is a serious restriction. Usually you want to print the most generic docstring first. Then say that for specific types the behaviour is as follows. But with the proposed change this would be impossible.

It must be possible to pull in only a specific dostring. For this reason, I think that Documenter should try to match only equal docstrings, no downwards or upwards. At the moment Documenter is trying to do exactly this, but it is confused in the presence of parametric types and some other situations.

@mortenpi
Copy link
Member

You are right -- we should only match exactly, not pull in multiple ones. As you say, it should be possible to splice both just general signatures and just specific signatures. For every line in an at-docs block, there should be a single docstring in the output.

I think we should define the at-docs block to be a "low level" way of splicing docstrings. So it should allow you to be precise at the expense of additional verbosity. If we want a feature for pulling in multiple docstrings based on some type matching rules, that should be a feature of e.g. at-autodocs.

As you say, this is how it appears to be working already. Just the parametric type thing needs to be fixed.

That said, right now it seems that, when there is no exact match available, Documenter will do the same "dispatch" as Julia. So, for foo(x::Float32) in the at-docs block it would show the foo(x::Real) docstring. I am wondering if we should consider that to be an error? Or at least print a warning?

@thofma
Copy link
Contributor Author

thofma commented Sep 28, 2018

I like the idea of letting at-docs blocks be the "low level" way of splicing dosstrings. Regarding what to do, if there is no signature equal to the one queried (like in the foo(x::Float32) example), I am not totally sure. At the moment it pulls in #1, #2 and #3. If we think of this as the "low level" way, then I guess that the only "correct" thing to do is to treat this as "No docs found for this signature".

I think foo (without signature) should pull in all docstrings.

The interesting question is, what should foo(::Union{}) do? All docstrings for foo or only the docstring for foo(::Union{}) (if it exists)? It is a bit of a strange signature, since it can never be called (by definition no object can have type Union{}).

@mortenpi
Copy link
Member

mortenpi commented Oct 2, 2018

If we think of this as the "low level" way, then I guess that the only "correct" thing to do is to treat this as "No docs found for this signature".

Yeah, I think that would be the way to do. I don't think this would affect that many builds either.. I find it unlikely that people would use this more specific type to specify their docstrings at the moment, so it would be a relatively harmless change to do.

I think foo (without signature) should pull in all docstrings.

Yup, just like should be doing right now.

The interesting question is, what should foo(::Union{}) do?

I would've said "just throw docstring not found", but you can actually define a method like that (even though it can not be called). So I'd say if the user defines a method like this and refers to it, it should just work.. even though it should never come up in practice.

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

2 participants