Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
feat: add test to detect public names without a docstring #313
Changes from all commits
e40aa41
fe267d3
0fdd06f
e036683
d4585c9
4df5e8b
05bf3ec
9e39dc9
2a3c4aa
83150f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 112 in src/Aqua.jl
Codecov / codecov/patch
src/Aqua.jl#L112
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 emptyThere was a problem hiding this comment.
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 thepublic
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 liketest_undocumented_api
,test_undocumented_public
or something else that include the word "public" without being as verbose astest_undocumented_public_names
.There was a problem hiding this comment.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the filter? It's good practice for a module to have a docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context: #313 (comment)