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

Fix redundant/broken intradoc links and minor cleanup #716

Merged
merged 13 commits into from
May 20, 2024

Conversation

MarijnS95
Copy link
Contributor

After spotting a missing closing parenthesis in [`EventQueue::blocking_dispatch()`](EventQueue::blocking_dispatch, leading to the link not rendering correctly, I set out on a course to delete these unnecessary link targets as [`EventQueue::blocking_dispatch()`] on its own already translates to the expected doc link, without being verbose. At the same time this PR includes a smattering of improvements, such as fixing missing links and updating mentions of functions that have been renamed or moved.

Finally, this PR sets up an extra CI step to make sure documentation doesn't have any build errors going forward.

I don't think this PR should be merged as-is, before we set up a little style guide for docs and finish the remainder. There's a bit of a mix currently but nothing we can't fix, so I'd like to ask the maintainers for their preference/agreement:

  • Convert all []() links to [][], as the former converts into a nonsense URL sometimes when the target is not a valid item and there's no matching /// [bar]: ... label target in scope, whereas the latter fails the build appropriately (as described by me in On wasm, provide intradoc-link for spawn() function in EventLoop docs rust-windowing/winit#3178 (comment));
  • Links to functions should use () in the name to avoid ambiguity;
  • Link text that points to a different destination (i.e. [`foo()`][Self::foo()]) should use parenthesis in the target as well, again to avoid ambiguity;
  • Links to functions on the current "scope" i.e. on Self, should be named as:
    • [`FullStructName::func()`];
    • [`func()`][FullStructName::func()];
    • [`func()`][Self::func()];
  • Links with crate in them (like [`crate::event_created_child!()`]) should be avoided and instead use an explicit link target.

Thanks for considering this! The documentation is already great and I hope this makes it more consistent (and less likely to reference (re)moved items) going forward!

@elinorbgr
Copy link
Member

Thanks a lot for this work, your suggestions seem quite good. 👍

I honestly don't have any strong feelings about the precise documentation style we use, but I agree that having a consistent one would absolutely be a good thing. Do you have any thoughts on that @ids1024 ?

@MarijnS95
Copy link
Contributor Author

Thanks @elinorbgr! Since proposing that list I've made a few picks already, bit the bullet, and pushed the changes to consistenize the docs. Only removing crate:: on macros remains, and do point it out to me if I forgot a link/comment or two.

There were a few non-doc foo() references already, there are probably more to find that should be turned into proper links before marking this as finished.

@ids1024
Copy link
Member

ids1024 commented May 4, 2024

Convert all links to [][], as the former converts into a nonsense URL sometimes when the target is not a valid item and there's no matching /// [bar]: ... label target in scope, whereas the latter fails the build appropriate

I didn't know [][] links like that were a thing, but that sounds like a good reason to use them. Broken links like that can be a common problem in Rust documentation.

Copy link
Member

@ids1024 ids1024 left a comment

Choose a reason for hiding this comment

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

The changes here all look good to me. I also like having CI check for cargo doc warnings like this.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Contributor Author

Since I have rebased, I should probably check if there are any new docs merged since that time that need touch-ups. And maybe set the "style guideline" in stone somewhere?

@MarijnS95
Copy link
Contributor Author

It wasn't too much but there's one point of confusion that I outlined here: https://github.com/Smithay/wayland-rs/pull/680/files#r1592423992

By at least pushing an intradoc link from get_cursor() to set_callback() (which is only mentioned as "fallback" and not "callback"), it should be easier to link the two together for users.

@MarijnS95
Copy link
Contributor Author

Looks like nightly in the CI immediately picked up on https://blog.rust-lang.org/2024/05/06/check-cfg.html, for which I've filed #722 to address this separately.

@ids1024
Copy link
Member

ids1024 commented May 14, 2024

Presumably CI will pass if this is rebased.

@MarijnS95
Copy link
Contributor Author

@ids1024 it will, though I'd like #723 to be merged first to get rid of some of the unrelated changes in this PR.

…d parenthesis

Keeping the link target around is redundant as `rustdoc` already infers
this from the link name, after removing the backticks.  It uses the
call-parenthesis to enforce that the link target is indeed an `fn`
(and not some other item).
…y_ptr()`

This moved for the server but not the client `Backend`, and because of
not being a link nor validated in the CI this broken reference wasn't
noticed.
Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.93%. Comparing base (1f6f4f5) to head (06532c9).
Report is 8 commits behind head on master.

Current head 06532c9 differs from pull request most recent head 3797bbb

Please upload reports for the commit 3797bbb to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #716      +/-   ##
==========================================
+ Coverage   73.11%   75.93%   +2.81%     
==========================================
  Files          45       46       +1     
  Lines        8183     8197      +14     
==========================================
+ Hits         5983     6224     +241     
+ Misses       2200     1973     -227     
Flag Coverage Δ
main 58.92% <ø> (+<0.01%) ⬆️
test--server_system 64.72% <100.00%> (?)
test-client_system- 72.23% <100.00%> (?)
test-client_system-server_system 55.05% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented May 16, 2024

Looks like a local Rust 1.78 run shows quite a few more issues now 🎉

EDIT: The CI here doesn't see them, but I've fixed it anyway. Fortunately most of the dead links locally were caused by the missing server_system feature, which is enabled via all-features = true on docs.rs at least!

@ids1024 ids1024 merged commit 531dee4 into Smithay:master May 20, 2024
12 checks passed
@MarijnS95 MarijnS95 deleted the docs branch May 21, 2024 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants