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

Refactor getdocs code #1842

Merged
merged 8 commits into from
Jun 14, 2022
Merged

Conversation

mgkurtz
Copy link
Contributor

@mgkurtz mgkurtz commented Jun 5, 2022

The current implementation of DocSystem.getdocs means, that in the case of

"A"
struct A end
"A(x)"
A(x) = A()
B = A

if I ask for the docs of B as in

```@docs
B
```

I get both "A" and "A(x)". Also see the second paragraph here. This PR makes the fallback search order for docs more explicit and would in this example give "A" only.

@mortenpi
Copy link
Member

So, just to make sure I understand, the problem we currently have is that if I ask for the docs of B I get

julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B), Union{})
2-element Vector{Base.Docs.DocStr}:
 Base.Docs.DocStr(svec("A"), nothing, Dict{Symbol, Any}(:typesig => Union{}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[2]", :fields => Dict{Symbol, Any}()))
 Base.Docs.DocStr(svec("A(x)"), nothing, Dict{Symbol, Any}(:typesig => Tuple{Any}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[3]"))

rather than the same as in

julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :A), Union{})
1-element Vector{Base.Docs.DocStr}:
 Base.Docs.DocStr(svec("A"), nothing, Dict{Symbol, Any}(:typesig => Union{}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[2]", :fields => Dict{Symbol, Any}()))

? If that's the intent, then I agree, we indeed shouldn't relax the constraint.

However, it looks like it doesn't return the correct thing right now?

julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B))
1-element Vector{Base.Docs.DocStr}:
 Base.Docs.DocStr(svec("A(x)"), nothing, Dict{Symbol, Any}(:typesig => Tuple{Any}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[3]"))

julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B), Union{})
1-element Vector{Base.Docs.DocStr}:
 Base.Docs.DocStr(svec("A(x)"), nothing, Dict{Symbol, Any}(:typesig => Tuple{Any}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[3]"))

julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B), Tuple{Any})
1-element Vector{Base.Docs.DocStr}:
 Base.Docs.DocStr(svec("A(x)"), nothing, Dict{Symbol, Any}(:typesig => Tuple{Any}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[3]"))

Either way, would you mind adding some tests to test/docsystem.jl for this?

@mgkurtz
Copy link
Contributor Author

mgkurtz commented Jun 10, 2022

Yes, the problem is that due to the previous implementation, asking for the docs of B for some signature (using ==) resulted in accessing the docs of A using <: (where I would have expected ==).

Now I have extracted the main part of getdocs in an own method and only the fallback logic remains in getdocs itself. As far as I see, no code in Documenter really uses the compare and aliases keyword arguments to getdocs. Do you know of some other packages, which rely on this? Otherwise we may get rid of them, simplifying the code some more.

However, it looks like it doesn't return the correct thing right now?

Strange, for me it correctly gives

julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B))                                                                                                                                                                                                               
1-element Vector{Base.Docs.DocStr}:                                                                                                                                                                                                                                       
 Base.Docs.DocStr(svec("A"), nothing, Dict{Symbol, Any}(:typesig => Union{}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[1]", :fields => Dict{Symbol, Any}()))                                                                                       
                                                                                                                                                                                                                                                                          
julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B), Union{})                                                                                                                                                                                                      
1-element Vector{Base.Docs.DocStr}:                                                                                                                                                                                                                                       
 Base.Docs.DocStr(svec("A"), nothing, Dict{Symbol, Any}(:typesig => Union{}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[1]", :fields => Dict{Symbol, Any}()))                                                                                       
                                                                                                                                                                                                                                                                          
julia> Documenter.DocSystem.getdocs(Docs.Binding(Main, :B), Tuple{Any})                                                                                                                                                                                                   
1-element Vector{Base.Docs.DocStr}:                                                                                                                                                                                                                                       
 Base.Docs.DocStr(svec("A(x)"), nothing, Dict{Symbol, Any}(:typesig => Tuple{Any}, :module => Main, :linenumber => 1, :binding => A, :path => "REPL[2]"))                                                                                                                 

Either way, would you mind adding some tests to test/docsystem.jl for this?

Right, I should have done this. Now there is one.

* stick to Documenter's binding, for consistency (at-var is not
  documented)
* Be explicit about the type signature
* Add, rather than replace tests
* Add separate test with the example from PR (also, covers the case of
  types)
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I am not sure why I got the output that I did yesterday.. I tested the old commit again, and it was fine now. My Julia session must have been in some weird state.

I took the liberty of rearranging the tests a bit and also adding the CHANGELOG. As far as I can tell, this only change behaviour when aliases are involved, and in that case we're currently doing the wrong thing. So I think we can have this as a bugfix in a patch release.

If the updates look good to you as well, then I think this is now good to go.

@mortenpi mortenpi added this to the 0.27.20 milestone Jun 14, 2022
@mortenpi
Copy link
Member

I'll go ahead and merge this. Thanks @mgkurtz!

@mortenpi mortenpi merged commit 373590a into JuliaDocs:master Jun 14, 2022
@mgkurtz
Copy link
Contributor Author

mgkurtz commented Jun 15, 2022

Thanks a lot for your additions and for merging 😊

mortenpi pushed a commit that referenced this pull request Jul 6, 2022
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>

(cherry picked from commit 373590a)
@mortenpi mortenpi mentioned this pull request Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants