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

Update README with feature flags examples #458

Merged
merged 7 commits into from
Jun 6, 2023

Conversation

staniewzki
Copy link
Collaborator

@staniewzki staniewzki commented May 30, 2023

I am not sure if adding this to FAQ is the best approach, especially as this section is pretty big relative to the others.

There's also a slight change in a comment that I didn't commit in the last PR on accident.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! Writing user-facing docs is really hard and takes a lot of practice and iterations to get to the highly-polished state that is often seen in the Rust ecosystem. I've made some suggestions to move closer to that state.

README.md Outdated
Comment on lines 80 to 102
```
--default-features
Use only the crate-defined default features, as well as any features added explicitly via other flags.

Using this flag disables the heuristic that enables all features except `unstable`, `nightly`, `bench`, `no_std`, and ones starting with `__`.

--only-explicit-features
Use no features except ones explicitly added by other flags.

Using this flag disables the heuristic that enables all features except `unstable`, `nightly`, `bench`, `no_std`, and ones starting with `__`.

--features <NAME>
Add a feature to the set of features being checked. The feature will be used in both the baseline and the current version of the crate

--baseline-features <NAME>
Add a feature to the set of features being checked. The feature will be used in the baseline version of the crate only

--current-features <NAME>
Add a feature to the set of features being checked. The feature will be used in the current version of the crate only

--all-features
Use all the features, including features named `unstable`, `nightly`, `bench`, `no_std` or starting with `__`, that are otherwise disabled by default
```
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is little bit awkward. Some solutions I see:

  1. Try to wrap the lines to make them shorter. This still results in a big code block.
  2. Move it out of a codeblock, and briefly explain all the flags related to features.
  3. Scrap it altogether, and explain the functionality in a shorter way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I implemented the 2. solution, and I think I like it.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! Writing user-facing docs is really hard and takes a lot of practice and iterations to get to the highly-polished state that is often seen in the Rust ecosystem. I've made some suggestions to move closer to that state.

@obi1kenobi
Copy link
Owner

obi1kenobi commented May 31, 2023

@staniewzki do you think you'd be able to wrap this up in the next couple of days? If not, just let me know and I can take it over.

Rust 1.70 comes out tomorrow and it's a good time to ship our own new release with explicit 1.70 support and dropping 1.65/1.66. It would be great to have the docs up in time for the new version so they sync to crates.io.

@staniewzki
Copy link
Collaborator Author

staniewzki commented May 31, 2023

@staniewzki do you think you'd be able to wrap this up in the next couple of days? If not, just let me know and I can take it over.

Yes, I hope to finish this up quickly! I've already started working on your comments.

@staniewzki
Copy link
Collaborator Author

I have notices that in #463 the help messages were not updated, so I am adding that here.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

I like this! Just a few very minor comments and this is good to go.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 94 to 99
| none | `std`, `alloc`, `derive`, `rc` |
| `--features unstable` | `std`, `alloc`, `derive`, `rc`, `unstable` |
| `--all-features` | `std`, `alloc`, `derive`, `rc`, `unstable` |
| `--default-features` | `std` |
| `--default-features --features derive` | `std`, `derive` |
| `--only-explicit-features --features unstable` | `unstable` |
Copy link
Owner

Choose a reason for hiding this comment

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

This is much clearer already.

Have you thought about possibly adding an "explanation" column to the right?

For example:

  • In the none row it could say: "Feature unstable is excluded by the default heuristic."
  • In the next row it could say: "The flag explicitly adds unstable to the heuristic's selections."
    etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is a good idea, unfortunately now there are line breaks in the flags, but I don't think that is a big issue.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, it's unfortunate but definitely the lesser problem, this is fine as-is

README.md Outdated Show resolved Hide resolved
staniewzki and others added 2 commits June 6, 2023 13:35
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
@obi1kenobi obi1kenobi merged commit 3754640 into obi1kenobi:main Jun 6, 2023
@obi1kenobi
Copy link
Owner

Nicely done! I'm going to prep a release shortly.

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