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

add a way to test whether exported symbols are documented #51174

Closed
stevengj opened this issue Sep 4, 2023 · 11 comments · Fixed by #52413
Closed

add a way to test whether exported symbols are documented #51174

stevengj opened this issue Sep 4, 2023 · 11 comments · Fixed by #52413
Labels
docsystem The documentation building system good first issue Indicates a good issue for first-time contributors to Julia kind:feature Indicates new feature / enhancement requests testsystem The unit testing framework and Test stdlib

Comments

@stevengj
Copy link
Member

stevengj commented Sep 4, 2023

As discussed on discourse, it would be nice to have a (documented) way to test whether all exported symbols in a module have docstrings. Currently, this seems to require undocumented internals?

Something like:

# hasdoc should go in Base.Docs; might also be accomplished by refactoring Base.Docs.doc somehow
using  Base.Docs: Binding, defined, getdoc, resolve, meta, modules, aliasof
hasdoc(mod::Module, sym::Symbol) = hasdoc(Base.Docs.Binding(mod, sym))
function hasdoc(binding::Binding, sig::Type = Union{})
    defined(binding) && !isnothing(getdoc(resolve(binding), sig)) && return true
    for mod in modules
        dict = meta(mod; autoinit=false)
        !isnothing(dict) && haskey(dict, binding) && return true
    end
    alias = aliasof(binding)
    return alias == binding ? false : hasdoc(alias, sig)
end

# possible function for the Test module
function check_documented(mod::Module)
    nodocs = filter!(sym -> !hasdoc(mod, sym), names(mod))
    isempty(nodocs) || error("missing docstrings in $mod: $nodocs")
end

For example, this catches some missing docstrings in Base and several standard libraries:

julia> check_documented(Base)
ERROR: missing docstrings in Base: [:CanonicalIndexError, :CapturedException, :InvalidStateException]

julia> check_documented(LinearAlgebra)
ERROR: missing docstrings in LinearAlgebra: [:ColumnNorm, :LAPACKException, :NoPivot, :RankDeficientException, :RowMaximum, :RowNonZero, :copy_transpose!]

julia> check_documented(Markdown)
ERROR: missing docstrings in Markdown: [Symbol("@doc_str"), Symbol("@md_str"), :html, :latex]

julia> check_documented(Pkg)
ERROR: missing docstrings in Pkg: [Symbol("@pkg_str"), :PKGMODE_MANIFEST, :PKGMODE_PROJECT, :PRESERVE_ALL, :PRESERVE_ALL_INSTALLED, :PRESERVE_DIRECT, :PRESERVE_NONE, :PRESERVE_SEMVER, :PRESERVE_TIERED, :PRESERVE_TIERED_INSTALLED, :Pkg, :PreserveLevel, :Registry, :UPLEVEL_MAJOR, :UPLEVEL_MINOR, :UPLEVEL_PATCH]
@stevengj stevengj added docsystem The documentation building system testsystem The unit testing framework and Test stdlib kind:feature Indicates new feature / enhancement requests good first issue Indicates a good issue for first-time contributors to Julia labels Sep 4, 2023
@jariji
Copy link
Contributor

jariji commented Sep 4, 2023

A docstring is the place for showing the interface of a function, regardless of whether the caller is inside or outside of the same module. It is useful both for writing callers and for modifying the function itself.

Therefore I prefer to check that all functions have docstrings, not just public ones.

@stevengj
Copy link
Member Author

stevengj commented Sep 5, 2023

Therefore I prefer to check that all functions have docstrings, not just public ones.

Sure, for people that want this, that could be a keyword to check_documented (mirroring the all keyword argument in names). Documenting exported symbols should be the minimum expectation, and would be a good default for a Test function.

The main thing is to export a hasdoc function, and then you can check whatever you want (e.g. require that all non-underscored symbols have docstrings, or all functions beginning with the letter "d").

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 5, 2023

This possibly should be an issue for https://github.com/JuliaTesting/Aqua.jl, which seems to be the goto place for better quality tests over what is in Test.

@Krastanov
Copy link

Documenter also has some checks along these lines that can be part of CI when strict=true is set. It might make sense to host that functionality there.

@jariji
Copy link
Contributor

jariji commented Sep 5, 2023

There is no way to tell programmatically if a function is API without testing whether it has documentation, and that will remain true after public is added. Determining whether a function is API is a sufficiently basic task that it seems more appropriate to include it with Julia.

@Seelengrab
Copy link
Contributor

Seelengrab commented Sep 7, 2023

See also #26919 and #31202. Both have a (now broken) script for this.

I think this should be something the Docs stdlib provides, not Test or Aqua. Clearly, there's a need for this in Base too.

@stevengj
Copy link
Member Author

stevengj commented Sep 8, 2023

Regardless of where the tests belong (Test, Aqua, etcetera), I think Base.Docs needs provide a hasdoc function or similar, because its implementation is closely tied to Base.Docs internals.

(There is also the principle that anything that is needed for julia itself belongs in Base or the stdlibs, and ensuring that exports are documented is clearly something we need for the JuliaLang/julia CI. Based on that principle, I think a test utility also belongs in Test, but we can argue that separately if it is controversial. To start with, it would be good to just have a PR to add a Base.Docs.hasdoc function.)

@jariji
Copy link
Contributor

jariji commented Nov 12, 2023

It seems like the hasdoc function can be added separately from any check_ functionality. @stevengj is your hasdoc ready for a PR?

@stevengj
Copy link
Member Author

stevengj commented Nov 12, 2023

Yes, someone should go ahead and make a PR with the hasdoc implementation and a test. Additional refinement can happen in the PR.

(That's why I marked it as "good first issue". You can start with the code above as-is.)

@jariji
Copy link
Contributor

jariji commented Dec 5, 2023

Not quite done - that PR adds Docs.hasdoc but we still need

function check_documented(mod::Module)
    nodocs = filter!(sym -> !hasdoc(mod, sym), names(mod))
    isempty(nodocs) || error("missing docstrings in $mod: $nodocs")
end

in Docs or Test. @Seelengrab can you explain why you prefer check_documented be in Docs rather than Test?

@Seelengrab
Copy link
Contributor

Why should Test depend on Docs for something that is purely Docs functionality? Either having that in Docs or in an extension of Test weakly depending on Docs would be fine IMO.

IanButterworth pushed a commit that referenced this issue Dec 31, 2023
Fixes #51174

---------

Co-authored-by: Steven G. Johnson <stevenj@alum.mit.edu>
Co-authored-by: Steven G. Johnson <stevenj@mit.edu>
stevengj added a commit that referenced this issue Jan 14, 2024
Following #52413 by @jariji and the discussion in #51174, this adds
tests to ensure that there are no undocumented *(= lacking docstrings)*
public symbols in any documented module.

Many of the tests are broken — we have a lot of undocumented exports. In
such cases I added both a `@test_broken` and a `@test` to ensure that
there are no additional regressions added in the future.

~~(This PR may have to be updated when
#52413 (comment) is
fixed, i.e. when ~~#52727~~ #52743 is merged.)~~ Has been updated.

---------

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docsystem The documentation building system good first issue Indicates a good issue for first-time contributors to Julia kind:feature Indicates new feature / enhancement requests testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants