feat: add test to detect public names without a docstring #313
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #313 +/- ##
==========================================
+ Coverage 74.90% 74.93% +0.03%
==========================================
Files 11 12 +1
Lines 761 774 +13
==========================================
+ Hits 570 580 +10
- Misses 191 194 +3 ☔ View full report in Codecov by Sentry. |
gdalle
changed the title
|
|
||
| !!! warning | ||
| For Julia versions before 1.11, this does not test anything. | ||
| """ |
There was a problem hiding this comment.
I would prefer seeing this warning in code with an @warn rather than a comment.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
the feature and implementation look good to me, but I think it is breaking to add a new default-on check (both on the principle that it would be very annoying to start failing CI "out of nowhere" and precedent, e.g. #174).
I think this could start off-by-default or just be v0.9.0.
| """ | ||
| function test_undocumented_names(m::Module) | ||
| @static if VERSION >= v"1.11" | ||
| undocumented_names = filter(n -> n != nameof(m), Docs.undocumented_names(m)) |
There was a problem hiding this comment.
| undocumented_names = filter(n -> n != nameof(m), Docs.undocumented_names(m)) | |
| undocumented_names = filter(n -> n != nameof(m), Docs.undocumented_names(m)) |
Why the filter? It's good practice for a module to have a docstring.
Either is fine by me. I guess it depends whether there are other pending breaking changes which might benefit from tagging v0.9.0 for this one. Also I'm worried that if we leave this test off at first we'll never remember to turn it on at the next breaking release. |
|
Personally I think doing a breaking release whenever there's a new check is alright, and just have all checks opt-out by default. I'm not an Aqua maintainer, but I've used aqua in a lot of repos with varying levels of activity, and I haven't found breaking releases of Aqua particularly annoying. No other packages (should) depend on Aqua so being on an old release doesn't hold back other packages, and it's fine to just stay on an older release until the next maintenance pass. I think mixing opt-in and opt-out is a bit more confusing and requires more attention for users. |
|
Tentatively bumping the version to v0.9.0, we'll see what the maintainers think |
I think the whole point of semver falls to bits if packages start cutting corners on what is and what is not breaking. I'm not sure what I usually see happening in practice, what kind of compat (caret, fixed, free) package developers have with their Aqua (test) dependency. A suggestion might be that Aqua warns users of newer versions, without actually breaking anything. Or that the documentation recommends dependents do not use a compat for Aqua, to avoid Aqua-clearance giving a false sense of security that all is well. |
|
|
||
| !!! warning | ||
| For Julia versions before 1.11, this does not test anything. | ||
| """ |
There was a problem hiding this comment.
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.
|
LGTM |
|
@lgoettgens would you mind taking a look? I got several reviews but none of them from maintainers I think? |
|
gentle bump on this one, perhaps @fingolfin would be open to reviewing? |
|
I am very busy right now, but I'll try to squeeze looking at this in sometime the upcoming week or the one after. Sorry for not having time earlier for a proper review |
|
No worries, I completely understand. As a maintainer I know that things sometimes slip under the radar so I thought I'd bump, but there is no rush. |
Fixes #90
Source:
test_undocumented_nameswhich checks whether public names are documented or not (only on Julia 1.11 and later). It does so by verifying thatDocs.undocumented_names(module)is either empty or contains only the module name itself.Tests:
test_undocumented_namestotest_allDocs:
test_undocumented_namesChores: