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

Put commands in their own files and some stricter lints #25

Merged
merged 4 commits into from
Dec 15, 2024

Conversation

tombh
Copy link
Contributor

@tombh tombh commented Dec 14, 2024

There are 2 commits. The first just splits up main.rs so that each command has its own file. And the second commit adds some stricter lints and makes them happy, most of which involves adding documentation. So it's worth reviewing each commit separately to get a better idea of the changes. I'm not 100% I have the best understanding of some things, so some of the comments I've added or amended might not be correct.

The new lints are quite a bit stricter, which might not be the most appropriate for this repo. Maybe they're overkill? The only one I think is important is the missing_docs = "warn" one.

Copy link
Collaborator

@schell schell left a comment

Choose a reason for hiding this comment

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

Great changes! I only had one nitpicky change request about the name of the "builder" module, but my opinion on it is not that strong 👍

crates/cargo-gpu/src/main.rs Outdated Show resolved Hide resolved
@@ -1,8 +1,8 @@
name: push

on:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised my editor is leaving this whitespace!

/// spirv-builder dependency, written just like in a Cargo.toml file.
#[clap(long, default_value = Spirv::DEFAULT_DEP)]
spirv_builder: String,

/// Rust toolchain channel to use to build `spirv-builder`.
///
/// This must match the `spirv_builder` argument.
/// This must be compatible with the `spirv_builder` argument as defined in the `rust-gpu` repo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

for entry in std::fs::read_dir(&release).unwrap() {
let entry = entry.unwrap();
for maybe_entry in std::fs::read_dir(&release).unwrap() {
let entry = maybe_entry.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 good naming :)


for sub in cmd.get_subcommands_mut() {
let _ = writeln!(buffer);
writeln!(buffer).unwrap();
#[expect(clippy::used_underscore_binding, reason = "Used in recursion only")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've never seen this macro/annotation before (expect) - cool :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really new actually, like just became stable a few months ago.

@tombh
Copy link
Contributor Author

tombh commented Dec 15, 2024

There's 2 new commits now.

The first new one adds an actual build.run() to the smoke tests? I wonder you didn't add that before? For speed maybe?

And the second new commit is just cos I want to rebase it into the first commit before merging the PR. I like to try for linear history.

@schell
Copy link
Collaborator

schell commented Dec 15, 2024

I wonder you didn't add that before? For speed maybe?

I was juggling a couple projects at once and using one of those to debug this one, and simply wasn't as thorough as I should have been, but luckily we've got @tombh to pick up the slack, thanks!

Awesome! Thanks for this.

@schell schell merged commit e4b1ad8 into Rust-GPU:main Dec 15, 2024
4 checks passed
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