Skip to content

Fix Issue 14622 - documented unit test following undocumented symbol #14527

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Fix Issue 14622 - documented unit test following undocumented symbol #14527

wants to merge 11 commits into from

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Oct 5, 2022 •
edited
Loading

Add diagnostic.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 14622 enhancement documented unit test that follows an undocumented symbol silently ignored

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14527"

Geod24
Geod24 reviewed Oct 5, 2022
View reviewed changes
compiler/src/dmd/parse.d Outdated
Comment on lines 494 to 499
if (pAttrs.comment && *pLastDecl && !pLastDecl.comment)
{
error("Documented unittest found following undocumented symbol `%s`",
pLastDecl.toPrettyChars());
errorSupplemental(pLastDecl.loc, "`%s` declared here", pLastDecl.toChars());
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's not error out on documentation unless we are generating documentation.

@Geod24
Copy link
Member

Geod24 commented Oct 5, 2022

Looking at this from another PoV, why aren't public symbol put in the documentation regardless of their /// ?
I use /// everywhere because that IMO should be the default behavior.

ntrel added 5 commits October 5, 2022 16:58
@ntrel
Ignore if previous symbol is an undocumented unittest
c603c54 
There's no way to get the matching doc symbol from the previous
unittest.
@ntrel
Fix compilable example
461be3d
@ntrel
Only error with -D
4804d3d
@ntrel
Fix detecting documented UT after undocumented UT; add rationale
89f79e7 
Move check to doc.d.
Also remove unnecessary compile check that is already in `compilable`.
@ntrel
Found undocumented AssocArray with documented test!
807e552
@ntrel
Copy link
Contributor Author

ntrel commented Oct 6, 2022 •
edited
Loading

@Geod24 For general symbols: sometimes you might write documentation for a symbol and then decide it's not ready, so you remove the doc comment. For nested doc comments inside that symbol it would be awkward to go through and undocument them, then when it's ready go through it again and remember to re-document all of them properly. So it can be useful that such nested ddoc comments are ignored.

Documented unit tests are different because they are easier to accidentally break, I explained why in a comment in 89f79e7.

ntrel
ntrel commented Oct 9, 2022
View reviewed changes
druntime/src/core/thread/osthread.d
return cast(Thread) cast(void*) t;
}

private extern(D) static void thread_yield() @nogc nothrow
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is private and not used anywhere in this module. Presumably I should remove it?

ntrel added a commit to ntrel/phobos that referenced this pull request Oct 15, 2022
@ntrel
Fix missing staticArray unittest in docs
6206161 
Found using dlang/dmd#14527.
@ntrel ntrel mentioned this pull request Oct 15, 2022
Merged
@ntrel
Copy link
Contributor Author

ntrel commented Oct 15, 2022

The buildkite ae failure seems unrelated:

std.socket.SocketOSException@std/socket.d(2826): Unable to bind socket: Cannot assign requested address
 

thewilsonator pushed a commit to dlang/phobos that referenced this pull request Oct 16, 2022
@ntrel
Fix missing unittests in docs (#8603)
281f42d 
* Fix missing staticArray unittest in docs

Found using dlang/dmd#14527.

* Fix missing FloatRep and DoubleRep unittests in docs

* 2 JSONValue op overloads

* std.random

* detabber

* remove

* std.digest.murmurhash
@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 1, 2022

ae doesn't fail anymore, however, we have the documentation tester failing:

/home/dtest/DAutoTest/work/tmp/ddoc_preprocessor_64396295aa5d4f108caabf7e23507f68/std/file.d(1985): Error: Documented unittest found following undocumented symbol `existsImpl` /home/dtest/DAutoTest/work/tmp/ddoc_preprocessor_64396295aa5d4f108caabf7e23507f68/std/file.d(1947):        `existsImpl` declared here

which is valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Geod24 Geod24 Geod24 left review comments

Assignees
No one assigned
Labels
Merge:needs rebase Review:Needs Rebase Review:Needs Work Review:stalled Severity:Enhancement stalled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ntrel @dlang-bot @Geod24 @RazvanN7