Skip to content

Update some documentation #233

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

Merged
merged 6 commits into from
Apr 14, 2020
Merged

Update some documentation #233

merged 6 commits into from
Apr 14, 2020

Conversation

hannobraun
Copy link
Member

Also contains a non-documentation fix that didn't seem substantial enough to put in its own PR.

@david-sawatzke I'd like to re-iterate my opinion that we should add the core API for each type as inherent methods, and only provide the embedded-hal traits in addition. Please take a look at the documentation of USART, usart::Rx, usart::Tx, and GpioPin.

It's totally not obvious what kind of API they provide. The relevant trait implementations are hidden in a list of stuff that is irrelevant to the user. Even if you know what you're looking for, you have to click on a [+] twice, before a method signature shows up.

I've worked around this by adding links to the relevant traits to the documentation of each of those structs, but those links now have to be maintained and will soon be out of date. And they are still not as obvious as method being where everyone expects them to be.

I'm more convinced than ever that the inherent methods should be there, even if they are complete duplicates of trait methods.

@david-sawatzke
Copy link
Member

david-sawatzke commented Apr 10, 2020

I still think that the proposed cure would be worse than the disease (duplicated functions being confusing, ugly hack in api for improving generated documentation, not obviously coming from an embedded-hal trait).

There are a couple things to improve the situation apart from duplicating the traits, solely for improving the documentation:

  • Make the embedded-hal impls in the introductory section more obvious (in my case with a subheading), succinctly explain what each trait is for
  • To minimize/eliminate link breakage, use anchor links to the trait on the same page
  • To avoid one click on [+], copy the documentation, so its not under the 'Show hidden undocumented items' section (see rustdoc: some trait methods appear as "hidden undocumented items" rust-lang/rust#56546)
    (You can avoid the remaining click on [+] by disabling the "auto-hide trait" stuff in the settings)

I think this is better than simply duplicating the methods, since it puts the "core functionality" (embedded-hal impls) front and center, and, to be honest, I'm quite happy with the result.

I've implemented it here, the final result looks like this:
Screenshot_2020-04-10 lpc8xx_hal usart USART - Rust
Screenshot_2020-04-10 lpc8xx_hal usart USART - Rust(1)

Hope you're happy with that as well, since I can't think of anything else.

@hannobraun
Copy link
Member Author

Okay, first off, I'm merging this now, since it was approved and additional improvements can be made later.

Second, I like your suggestions, and I've made a note to look into them on my next round of documentation updates.

Third, if what you suggest here is what we end up with, I'm fine with that. However, I feel the need to defend my suggestion (again), because I think you are misrepresenting it in your comment.

I still think that the proposed cure would be worse than the disease (duplicated functions being confusing, ugly hack in api for improving generated documentation, not obviously coming from an embedded-hal trait).

It's not a hack to improve generated documentation, that's just one of the benefits. The other benefit (which I didn't mention here, as this PR is about documentation) is that you don't have to import the prelude everywhere, which I find to be a constant low-level annoyance.

I also still don't understand what would be so bad about it (or rather, why it would be bad at all). It's not like we're duplicating some fragile code that's sure to break at some point. The additional method would consist of a single, fully type-checked method call. Any confusion that might arise is easily alleviated by a note in the documentation. And we already have proxy methods on USART, which you approved, if I recall correctly.

Honestly, I'm quite baffled at how much this whole concept seems to disgust you 🙂

@hannobraun hannobraun merged commit 46f7918 into lpc-rs:master Apr 14, 2020
@hannobraun hannobraun deleted the documentation branch April 14, 2020 07:55
david-sawatzke added a commit to david-sawatzke/lpc8xx-hal that referenced this pull request Apr 17, 2020
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.

2 participants