-
Notifications
You must be signed in to change notification settings - Fork 17
Improve README, pass 1 #405
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
base: main
Are you sure you want to change the base?
Conversation
Now ordered in the way you would use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only two blocking concerns, the rest are nits. This is a definite improvement, though!
README.md
Outdated
The **linter** currently needs to be installed separately with a specific nightly toolchain: | ||
|
||
The default logging level for the CLI is set to `info`. To change the log level set the `BEVY_LOG` environment variable. | ||
```sh | ||
# Install the toolchain | ||
rustup toolchain install nightly-2025-04-03 \ | ||
--component rustc-dev \ | ||
--component llvm-tools-preview | ||
|
||
# Install bevy_lint | ||
cargo +nightly-2025-04-03 install \ | ||
--git https://github.com/TheBevyFlock/bevy_cli.git \ | ||
--tag lint-v0.3.0 \ | ||
--locked \ | ||
bevy_lint | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hesitant on adding installation instructions for the linter here, as it can change fairly frequently. We can add notes to the toolchain upgrade guide and release guide mentioning that this needs to be updated, but it's not my favorite solution.
Instead, could you direct readers to the installation section of the linter's README.md
? When we add support for installing the linter through the CLI, we can add that here as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still in heavy draft, but made me start it #406 (;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry is that some users just won't bother when it's too hard to figure out.
Right now you first have to click the link to the linter README, then figure out the correct toolchain and git tag and then assemble the command and execute it.
Compared to just copy & pasting these commands that's a lot more work (even if it might be simple to do).
But yea, the automatic installation would be the ideal solution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I changed it back to just link to the linter documentation for now.
Can't wait for #406 though, that will improve the UX by a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new structure, it feels more natural to read!
README.md
Outdated
To use a specific template, provide the full GitHub URL | ||
|
||
```sh | ||
bevy new my-project -t https://github.com/TheBevyFlock/bevy_new_2d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if it would make sense here to take a Repository that is not from the TheBevyFlock
org to really show that it's "just" cargo generate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, since if someone wants to use bevy_new_2d
they'd just say -t 2d
. Would bevyengine/bevy_github_ci_template
work instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restructured this a bit to first introduce the short form and then the full URLs.
Also replaced the link with the one you suggested, although that template will need to be updated for the CLI.
README.md
Outdated
To use a specific template, provide the full GitHub URL: | ||
|
||
```sh | ||
bevy new my-project -t https://github.com/bevyengine/bevy_github_ci_template.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repository does not work:
❯ bevy new my-project -t https://github.com/bevyengine/bevy_github_ci_template.git -v
[ 3/17] Done: .github/workflows [ 4/17] Done: .github [ 5/17] Done: .gitignore [ 6/17] Done: Cargo.toml [ 7/17] Done: LICENSE-Apache-2.0 [ 8/17] Done: LICENSE-CC0-1.0 [ 9/17] Done: LICENSE-MIT [10/17] Done: README.md [12/17] Done: assets[13/17] Done: src/main.rs [14/17] Done: src [15/17] Done: wasm/index.html [16/17] Done: wasm/restart-audio-context.js [17/17] Done: wasm Error: Substitution skipped, found invalid syntax in
.github/workflows/ci.yaml
.github/workflows/release.yaml
assets/ducky.png
Consider adding these files to a `cargo-generate.toml` in the template repo to skip substitution on these files.
Learn more: https://github.com/cargo-generate/cargo-generate#include--exclude.
/tmp
❯ ls my-project
I think we either need to be able to handle this (treat repos that fail with cargo-generate as "normal" repos and just clone them) or state that only these repositories work, that work for cargo-generate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! I reverted back to bevy_new_2d
until we have a better option.
The other repository did not have cargo generate support and failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this. For a follow-up PR, I think the most important part is "how and what" can be configured. Otherwise, this looks good!
In preparation for a v0.1.0 release of the CLI, we need to improve the documentation.
This is a first pass to make the README more useful.