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

Improve README.md, add CONTRIBUTING.md, and more! #152

Closed
wants to merge 28 commits into from

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Oct 17, 2024

Closes #109.

This PR:

  • Creates bevy_lint's README.md
  • Modifies the main CLI's README.md
  • Creates and moves some information into CONTRIBUTING.md
  • Adds README.md into bevy_cli's rustdoc output.
  • Sets up some custom CSS styles so Github alerts render nicely in rustdoc.

image

Todo

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Docs An improvement in documentation labels Oct 17, 2024
@BD103 BD103 marked this pull request as ready for review October 18, 2024 03:29
@BD103
Copy link
Member Author

BD103 commented Oct 18, 2024

Unfortunately Github alerts are currently broken, but I'll take a look in the morning.

@BD103 BD103 changed the title Create README.md for bevy_lint Improve README.md, add CONTRIBUTING.md, and more! Oct 18, 2024
@BD103
Copy link
Member Author

BD103 commented Oct 18, 2024

So I tried using Github's alerts

Tip

like this

but they don't render correctly when wrappen in a <div>, turning into

[!CAUTION]

this :(

As such, I'm going to keep the pretty rustdoc alerts but replace Github's side

Note

With this. It's not nearly as nice, but I like it better than having no pretty alerts at all, or having rustdoc's plain block quotes.


at your option.

## Contributing
Copy link

Choose a reason for hiding this comment

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

Can we include some more help for contributors here? At the very least, I think it would be helpful to explain how to run the UI tests.

README.md Outdated
@@ -2,10 +2,25 @@

A Bevy CLI tool.
Copy link
Member

Choose a reason for hiding this comment

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

This needs more expansion to describe the current and planned features. This is the first thing people will read!

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure about what other planned features we have, so I just rewrote the opening description and added a list of current features. Tell me if there's anything I missed in fc10f2f!

First, you must install the toolchain and components described by [`rust-toolchain.toml`](https://github.com/TheBevyFlock/bevy_cli/blob/main/rust-toolchain.toml) using [Rustup]. As of the time of writing (October 17th, 2024), the command may look like this:

```bash
$ rustup toolchain install nightly-2024-10-03 \
Copy link
Member

Choose a reason for hiding this comment

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

How do users check that this is still the right version?

Copy link
Member Author

Choose a reason for hiding this comment

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

The version is specified in rust-toolchain.toml. A user would replace nightly-2024-10-03 with the value in the channel = "..." field.

This is a bit rough, I'll admit. How can I rephrase this to be more clear?

Copy link
Member

Choose a reason for hiding this comment

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

I think I would do:

First, you must install the exact toolchain expected by the version of the tool that you're using. You can determine the correct version by checking [rust-toolchain.toml]. For example, if you were using the commit HASH, you should look at...

bevy_lint/README.md Outdated Show resolved Hide resolved
bevy_lint/README.md Outdated Show resolved Hide resolved

### Configuring Lints

Toggling lints and lint groups requires the nightly `register_tool` feature. Even if your project uses stable Rust, you can still use this feature by detecting the `--cfg bevy_lint` flag:
Copy link
Member

Choose a reason for hiding this comment

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

This framing isn't clear to me. Start with a clear description of what the user is trying to do :)

}
```

If you do not register `bevy` as a tool, `#[allow(bevy::lint_name)]` and related attributes will fail to compile.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved up: motivation first.

unexpected_cfgs = { level = "warn", check-cfg = ["cfg(bevy_lint)"] }
```

You can now toggle lints and lint groups throughout the crate, as long as they are also behind `#[cfg_attr(...)]`:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can now toggle lints and lint groups throughout the crate, as long as they are also behind `#[cfg_attr(...)]`:
You can now toggle lints and lint groups throughout your project, as long as they are also behind `#[cfg_attr(...)]`:

Copy link
Member Author

Choose a reason for hiding this comment

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

I worry about saying this because you need to call #![register_tool(bevy)] for each crate root that you use the bevy tool namespace. This means all libraries, binaries, examples, and integration tests. Let me try thinking of a better way to communicate this...

@BD103 BD103 marked this pull request as draft October 20, 2024 03:21
@BD103
Copy link
Member Author

BD103 commented Oct 20, 2024

Thanks for the reviews! It clearly still needs a lot of work, so I'm going to pick this back up tomorrow. Cheers :)

@BD103
Copy link
Member Author

BD103 commented Oct 21, 2024

I'm going to split this up into a series of smaller PRs. It's too big already for my tastes, and some parts can get merged already while others need more work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Docs An improvement in documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write README.md for bevy_lint
3 participants