Skip to content

doc: massive overhaul of documentation #1264

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

Closed
wants to merge 13 commits into from
Closed

Conversation

phip1611
Copy link
Member

doc: reorganize

This is a massive re-organization of the existing documentation. The main goal
is to streamline what is provided in the README.md and what is in the lib.rs file.

I think, we need to answer some more questions and to redistribute
these answers to where users most naturally expect documentation. When in doubt,
lib.rs should be preferred and the README should forward to docs.rs.

This resolves also many inconsistencies along the way, remove (IMHO) unnecessary
pieces, and adds relevant (but yet missing) information.

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@phip1611 phip1611 requested a review from nicholasbishop July 28, 2024 13:38
@phip1611 phip1611 force-pushed the doc branch 2 times, most recently from 5d2c998 to 8a4a40a Compare July 28, 2024 13:49
@nicholasbishop
Copy link
Member

I'm broadly on board with these changes, but I'm having a little difficulty reviewing it all as one PR -- quite a bit of stuff is changing which makes it hard for me to feel confident I'm seeing all the details. Could we break some of this out into separate PRs, e.g. one PR to drop the out-of-date panic-on-logger-errors doc, one PR to update the uefi crate description, one PR to drop the license section from the uefi/README, etc?

(Small CLs is a summary I find helpful of why small changes can help reviews.)

@phip1611
Copy link
Member Author

phip1611 commented Jul 30, 2024

I'm broadly on board with these changes, but I'm having a little difficulty reviewing it all as one PR -- quite a bit of stuff is changing which makes it hard for me to feel confident I'm seeing all the details. Could we break some of this out into separate PRs, e.g. one PR to drop the out-of-date panic-on-logger-errors doc, one PR to update the uefi crate description, one PR to drop the license section from the uefi/README, etc?

(Small CLs is a summary I find helpful of why small changes can help reviews.)

Copy that, sorry. I'm usually totally on your side, but this time I was just a little too lazy. 🫣 🤣

Could we break some of this out into separate PRs

I don't think that multiple PRs help to transport the main idea and the higher objective of my changes. But I separated everything into multiple logical commits - I hope this improves the review-ability.. does it help?

I think the changes made here are a major improvement. As my boss always says: Do nice things and talk about it.

@phip1611 phip1611 force-pushed the doc branch 13 times, most recently from 0af5eea to 450bb33 Compare July 30, 2024 09:20
phip1611 added 3 commits July 30, 2024 11:22
The main idea here is to:
- tell that `uefi` is not only for EFI images but also apps that interact with
  UEFI functionality, such as parsing a memory map from a pre-defined chunk of
  memory.
- streamline the doc
This prepares what the following commits will provide.
This is the beginning of moving all relevant information from README.md to lib.rs.
The main idea is to have `lib.rs` as main entry point into the documentation instead
of an inconsistent mixture between the README and the lib.rs file.
@phip1611 phip1611 force-pushed the doc branch 4 times, most recently from c44e0ea to 903db8f Compare July 30, 2024 09:29
phip1611 added 3 commits July 30, 2024 11:34
Copy MSRV, License, and Contributing sections to lib.rs and streamline it
with the README equivalents. We should also include these information in
lib.rs, as it is the main entry point into everything relevant about the
library. At least, it should from now on.

The big benefit is that the documentation on `docs.rs` then covers
everything relevant.
The About section is a more comprehensive write-up of the TL;DR section
mentioned earlier. It should guide the user to know what they can do with
this lib.
phip1611 added 6 commits July 30, 2024 11:34
This is especially interesting as the `std` implementation of Rust is upcoming
…Resources"

This is logically following the About section.
Also added "UEFI Strings" subsection
Now, the README are the entry into the repository and guide the user to the
actual documentation in lib.rs respectively on docs.rs.
@nicholasbishop
Copy link
Member

I don't think that multiple PRs help to transport the main idea and the higher objective of my changes. But I separated everything into multiple logical commits - I hope this improves the review-ability.. does it help?

Yep, the multiple commits do help, thanks. I still think Github's UI for reviewing individual commits is not very good, so where possible I do prefer for parts to be pulled out to separate PRs when possible (in this case I think it wouldn't detract from anything to pull the panic-on-logger-errors removal into a separate PR, and to pull the Cargo.toml description change into a separate PR).

[![Crates.io](https://img.shields.io/crates/v/uefi)](https://crates.io/crates/uefi)
[![Docs.rs](https://docs.rs/uefi/badge.svg)](https://docs.rs/uefi)
![License](https://img.shields.io/github/license/rust-osdev/uefi-rs)
![Build status](https://github.com/rust-osdev/uefi-rs/workflows/Rust/badge.svg)
![Stars](https://img.shields.io/github/stars/rust-osdev/uefi-rs)

## TL;DR
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we should drop the tl;dr header here since the section is just one sentence. Have this content come directly after the "Rusty wrapper for the Unified Extensible Firmware Interface." sentence.

[![Crates.io](https://img.shields.io/crates/v/uefi)](https://crates.io/crates/uefi)
[![Docs.rs](https://docs.rs/uefi/badge.svg)](https://docs.rs/uefi)
![License](https://img.shields.io/github/license/rust-osdev/uefi-rs)
![Build status](https://github.com/rust-osdev/uefi-rs/workflows/Rust/badge.svg)
![Stars](https://img.shields.io/github/stars/rust-osdev/uefi-rs)

## TL;DR

Develop Rust software that leverages **safe**, **convenient**, and
Copy link
Member

Choose a reason for hiding this comment

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

nit: For clarity I would add a few words at the start of the sentence like "This crate makes it easy to develop Rust software [..].". Makes it clearer that we aren't necessarily the ones making the safe/convenient/performant software, but rather we're helping users of the crate do that.

@@ -12,6 +12,13 @@
//! Feel free to file bug reports and questions in our [issue tracker], and [PR
//! contributions][contributing] are also welcome!
//!
//! # About this Document
Copy link
Member

Choose a reason for hiding this comment

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

IMO this section isn't needed. Having some description at the front of a multi-page document can be helpful, but here it's pretty clear what all the content is by just scanning through the following sections on the page.

//! disks or the network. EFI images, the files that can be loaded by an UEFI
//! environment, can leverage these abstractions to extend the functionality in
//! form of additional drivers, OS-specific bootloaders, or any different kind
//! of low-level applications (such as an [IRC client][uefirc]).
Copy link
Member

Choose a reason for hiding this comment

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

The IRC client is a neat project, but I'm not sure it makes sense to mention in the readme since it's a pretty niche use case, unlike drivers and bootloaders.


## MSRV
With `uefi`, you have the flexibility to integrate selected types and
abstractions into your project or to conveniently create EFI images, addressing
Copy link
Member

Choose a reason for hiding this comment

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

nit: Here and elsewhere, let's standardize on always saying UEFI instead of EFI. I've heard people mention confusion around this on a few occasions, since it's not obvious without some research that we mean the same thing when we say EFI and UEFI.

//! create EFI images.
//!
//! We will closely monitor the situation and update the documentation
//! accordingly, once the `std` implementation is more mature.
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should not pass a judgement here on whether it's mature (parts are perfectly usable, parts are still in development, and it really just depends on what you need out of it).

I would drop everything from "that is yet far from being mature" on. Instead, just give the facts: explain that it allows you to write UEFI programs that look very similar to normal programs running on top of an OS, and also mention that it is still under development (without promising that we'll closely monitor it, since I think that monitoring is better done by potential users of it)

//! We will closely monitor the situation and update the documentation
//! accordingly, once the `std` implementation is more mature.
//!
//! ## `r-efi`
Copy link
Member

Choose a reason for hiding this comment

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

Add a link to the crate

@phip1611
Copy link
Member Author

Could you please drop a word or two on whether you think the overall changes are beneficial and a clear improvement? Do you support that lib.rs/docs.rs should be the main documentation entry into the project?

@nicholasbishop
Copy link
Member

Yes, I think removing some of the duplication, having a fairly minimal readme, and having lib.rs contain more details is good.

I also think there's a balance to be struck with documentation; it's OK to have some amount of duplication (https://docs.divio.com/documentation-system/structure/#the-tendency-to-collapse is a good doc on this). But I think the intent of our readme and lib.rs docstring are largely overlapping, so I think the direction of this PR is good.

@phip1611 phip1611 marked this pull request as draft August 1, 2024 05:57
@phip1611
Copy link
Member Author

phip1611 commented Aug 2, 2024

Split-out 1/N is live: #1284

@phip1611 phip1611 closed this Aug 6, 2024
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