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

feat: add test to detect public names without a docstring #313

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## Version [v0.8.10] - unreleased

### Changed

- Add `test_undocumented_names` to verify that all public symbols have docstrings (including the module itself). ([#313])


## Version [v0.8.9] - 2024-10-15

Expand Down
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "Aqua"
uuid = "4c88cf16-eb10-579e-8560-4a9242c79595"
authors = ["Takafumi Arakaki <[email protected]> and contributors"]
version = "0.8.9"
version = "0.8.10"

[deps]
Compat = "34da2185-b29b-5c13-b0c7-acf172513d20"
Expand Down
1 change: 1 addition & 0 deletions docs/make.jl
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ makedocs(;
"deps_compat.md",
"piracies.md",
"persistent_tasks.md",
"undocumented_names.md",
],
"release-notes.md",
],
Expand Down
4 changes: 4 additions & 0 deletions docs/src/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ Aqua.jl provides functions to run a few automatable checks for Julia packages:
* There are no "obvious" type piracies.
* The package does not create any persistent Tasks that might block precompilation of dependencies.

```@docs
Aqua
gdalle marked this conversation as resolved.
Show resolved Hide resolved
```

## Quick usage

Call `Aqua.test_all(YourPackage)` from the REPL, e.g.,
Expand Down
7 changes: 7 additions & 0 deletions docs/src/undocumented_names.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Undocumented names

## [Test function](@id test_undocumented_names)

```@docs
Aqua.test_undocumented_names
```
20 changes: 19 additions & 1 deletion src/Aqua.jl
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
"""
Aqua
gdalle marked this conversation as resolved.
Show resolved Hide resolved

Package providing Auto QUality Assurance for other Julia packages.

GitHub repository: https://github.com/JuliaTesting/Aqua.jl
"""
module Aqua

using Base: PkgId, UUID
using Base: Docs, PkgId, UUID
using Pkg: Pkg, TOML, PackageSpec
using Test

Expand All @@ -23,6 +30,7 @@
include("deps_compat.jl")
include("piracies.jl")
include("persistent_tasks.jl")
include("undocumented_names.jl")

"""
test_all(testtarget::Module)
Expand All @@ -37,6 +45,7 @@
* [`test_deps_compat(testtarget)`](@ref test_deps_compat)
* [`test_piracies(testtarget)`](@ref test_piracies)
* [`test_persistent_tasks(testtarget)`](@ref test_persistent_tasks)
* [`test_undocumented_names(testtarget)`](@ref test_undocumented_names)

The keyword argument `\$x` (e.g., `ambiguities`) can be used to
control whether or not to run `test_\$x` (e.g., `test_ambiguities`).
Expand All @@ -52,6 +61,7 @@
- `deps_compat = true`
- `piracies = true`
- `persistent_tasks = true`
- `undocumented_names = true`
"""
function test_all(
testtarget::Module;
Expand All @@ -63,6 +73,7 @@
deps_compat = true,
piracies = true,
persistent_tasks = true,
undocumented_names = true,
gdalle marked this conversation as resolved.
Show resolved Hide resolved
)
@testset "Method ambiguity" begin
if ambiguities !== false
Expand Down Expand Up @@ -105,6 +116,13 @@
test_persistent_tasks(testtarget; askwargs(persistent_tasks)...)
end
end
@testset "Undocumented names" begin

Check warning on line 119 in src/Aqua.jl

View check run for this annotation

Codecov / codecov/patch

src/Aqua.jl#L119

Added line #L119 was not covered by tests
if undocumented_names !== false
gdalle marked this conversation as resolved.
Show resolved Hide resolved
isempty(askwargs(undocumented_names)) ||
error("Keyword arguments not supported")
test_undocumented_names(testtarget; askwargs(undocumented_names)...)
end
end
end

end # module
26 changes: 26 additions & 0 deletions src/undocumented_names.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
"""
test_undocumented_names(module::Module)

Test that all public names in `module` have a docstring (including the module itself).
gdalle marked this conversation as resolved.
Show resolved Hide resolved

!!! warning
For Julia versions before 1.11, this does not test anything.
"""
Copy link

Choose a reason for hiding this comment

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

I would prefer seeing this warning in code with an @warn rather than a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That means adding a warning for every test below Julia 1.11, not sure people will like that. The fact that it does nothing on 1.10 can already be seen from the fact that the relevant @testset is empty

Copy link

@KeithWM KeithWM Nov 24, 2024

Choose a reason for hiding this comment

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

I realize now that this implementation relies heavily on the "new" public keyword, which was not actually in the original issue (as it predated the keyword). Given that a main reason for introducing the public keyword was that previously something being documented was used to show it was part of the API, I think the proposed implementation is a good idea. But I'm not quite sure how to deal with the expectations towards <1.11 users. It could help to change the name of the check to something like test_undocumented_api, test_undocumented_public or something else that include the word "public" without being as verbose as test_undocumented_public_names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that Docs.undocumented_names is not defined on 1.10, I'm not sure we can do anything meaningful before 1.11 (unless that function is added to Compat.jl)

function test_undocumented_names(m::Module)
@static if VERSION >= v"1.11"
names = Docs.undocumented_names(m)
@test isempty(names)
else
names = Symbol[]
end
if !isempty(names)
printstyled(
stderr,
"Undocumented names detected:\n";
bold = true,
color = Base.error_color(),
)
show(stderr, MIME"text/plain"(), names)
println(stderr)
end
end
20 changes: 20 additions & 0 deletions test/pkgs/PkgWithUndocumentedNames.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module PkgWithUndocumentedNames

"""
documented_function
"""
function documented_function end

function undocumented_function end

"""
DocumentedStruct
"""
struct DocumentedStruct end

struct UndocumentedStruct end

export documented_function, DocumentedStruct
export undocumented_function, UndocumentedStruct

end # module
18 changes: 18 additions & 0 deletions test/pkgs/PkgWithoutUndocumentedNames.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
"""
PkgWithoutUndocumentedNames
"""
module PkgWithoutUndocumentedNames

"""
documented_function
"""
function documented_function end

"""
DocumentedStruct
"""
struct DocumentedStruct end

export documented_function, DocumentedStruct

end # module
29 changes: 29 additions & 0 deletions test/test_undocumented_names.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module TestUndocumentedNames

include("preamble.jl")

import PkgWithUndocumentedNames
import PkgWithoutUndocumentedNames

# Pass
results = @testtestset begin
Aqua.test_undocumented_names(PkgWithoutUndocumentedNames)
gdalle marked this conversation as resolved.
Show resolved Hide resolved
end
if VERSION >= v"1.11"
@test length(results) == 1
@test results[1] isa Test.Pass
else
@test length(results) == 0
end
# Fail
results = @testtestset begin
Aqua.test_undocumented_names(PkgWithUndocumentedNames)
end
if VERSION >= v"1.11"
@test length(results) == 1
@test results[1] isa Test.Fail
else
@test length(results) == 0
end

end # module
Loading