Aqua.jl
Aqua.jl copied to clipboard
feat: add test to detect public names without a docstring
Fixes #90
Source:
- [x] Add a function
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:
- [x] Add a kwarg
test_undocumented_namestotest_all - [x] Add tests with one fully documented module and one partially documented module.
Docs:
- [x] Add docs page about
test_undocumented_names
Chores:
- [x] Bump version to v0.9.0 (BREAKING CHANGE)
- [x] Update CHANGELOG.
Codecov Report
Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
Project coverage is 75.09%. Comparing base (
3e90d04) to head (9c067ba). Report is 1 commits behind head on master.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/Aqua.jl | 25.00% | 3 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #313 +/- ##
==========================================
+ Coverage 74.90% 75.09% +0.19%
==========================================
Files 11 12 +1
Lines 765 783 +18
==========================================
+ Hits 573 588 +15
- Misses 192 195 +3
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
I think this could start off-by-default or just be v0.9.0.
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
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.
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.
Any update?
@lgoettgens from my end this is good to go
Sorry for the long time.
Needs a rebase in the meantime :-/ but hopefully an easy one.
Anything more needed here?
I would like to release this as a patch release with this test disabled by default to not disrupt the whole ecosystem, as there are many packages that don't declare a compat bound on Aqua.
What is the difference? One day there will be a breaking release switching this on, and people without a compat bound on Aqua will get surprised in exactly the same way.
Maybe we could add a warning telling people to switch to true for this setting, that way at least they catch it if they look at the test output? Otherwise I'm not sure anyone is actively monitoring release notes for Aqua's patch releases.