Add rustdoc to connect crate #1455
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 comment.
Files not reviewed (6)
- CHANGELOG.md: Evaluated as low risk
- connect/README.md: Evaluated as low risk
- connect/src/lib.rs: Evaluated as low risk
- connect/src/model.rs: Evaluated as low risk
- connect/src/state/metadata.rs: Evaluated as low risk
- src/main.rs: Evaluated as low risk
connect/src/model.rs
Outdated
| /// Represent an internal identifier from spotify. | ||
| /// | ||
| /// ## Remarks: | ||
| /// Is not intended for usage, but required for parsing of some connect messages. |
There was a problem hiding this comment.
This one could then use a little more explanation on how/when/why to use SpotifyId instead.
Or maybe you can #[no_doc] this particular one? Don't know if that works.
There was a problem hiding this comment.
I think I could improve the docs a bit to explicitly lay out that this should never used by the user as it's not an id that can be acquired anyways. No documentation is probably not helpful when using it, as you could use it in the wrong way, I could look into if I maybe find a way to hide the exposed Uid part tho...
So from what I understand is a uid are separate identifier to a uri and the id of a item (usually the third part of the uri). It's only used by the clients of spotify and never retrievable by public endpoints. Mobile uses it extensively and it's determines the fancy visual what track currently plays.
That's the background. I think I also documented something about it in docs/dealer.md.
SpotifyId itself is at most part's not inclusive enough, there are some URI's I found that you can't really parse with the current implementation of SpotifyId such as the user collection or the search (both require strings to be saved). But that's related to #1266 so maybe we could tackle that part at some point.
There was a problem hiding this comment.
Added #[doc(hidden)] to the variant and improved the documentation anyways, so that it still make some sense to other devs later down the line :D
connect/src/spirc.rs
Outdated
|
|
||
| /// Resumes the playback | ||
| /// | ||
| /// ## Remarks: |
There was a problem hiding this comment.
I don't think "Remarks" is an idiomatic Rustdoc paragraph? For me you could just drop the paragraph title and keep the paragraph text underneath it as-is.
There was a problem hiding this comment.
Hmm, I think I will do some research how to write better rustdoc and adjust the comments accordingly. The "Remarks" originates from a different language where you have separate section and it seems to fit as hint, but it could also be just a paragraph yeah :D
There was a problem hiding this comment.
I removed the "Remarks" sections and flattened it so that the remarks appear as paragraphs. There doesn't seem to be any real way to do it, besides the example sections which seemed to be pushed quite a bit.
|
When you're looking for anyone's review, you can specifically request / add them as reviewers and keep the assignee as yourself. |
Okay will do! You just said in another message that I should assigning you, but I suppose I mistook assigning to the PR with review assigning. |
|
Oh and big thanks for the review @roderickvd :D |
photovoltex
force-pushed
the
connect-docs
branch
2 times, most recently
from
fd6de96 to
bed00a6
Compare
photovoltex
force-pushed
the
connect-docs
branch
from
bed00a6 to
1416fd2
Compare
I think I initially tried to enforce rustdoc on the whole project, but that just is in insane task at this stage.
Well anyhow... I got around to add rustdoc for the connect crate and additionally a cleanup of the exposed items, so that only the necessary are exposed.
I also had to change up the
CHANGELOG.mdas some previous entries where are out of that with the cleanup changes.