Skip to content

Add cargo add instruction to crate-sidebar #5875

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

Merged
merged 2 commits into from
Jan 6, 2023

Conversation

stephank
Copy link
Contributor

@stephank stephank commented Jan 6, 2023

This adds a cargo add instruction to the crate sidebar, with clipboard support:

Screenshot 2023-01-06 at 20 07 01

This came about after @beeb made a good point that copying a snippet from crates.io prevents typosquatting, and not having a snippet for cargo add is reason to avoid it.

@beeb
Copy link

beeb commented Jan 6, 2023

Awesome feature! Thanks!

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 labels Jan 6, 2023
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

I'm wondering about adding the version to the command, but that probably doesn't need to block this PR. let me know if I should merge as is and then we can still improve this afterwards :)

@@ -14,6 +14,10 @@ export default class CrateSidebar extends Component {
return homepage && (!repository || simplifyUrl(repository) !== simplifyUrl(homepage));
}

get cargoAddCommand() {
return `cargo add ${this.args.crate.name}`;
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
return `cargo add ${this.args.crate.name}`;
return `cargo add ${this.args.crate.name}@${this.args.version.num}`;

any thoughts about including the version number, like we do for the Cargo.toml snippet?

I understand that cargo add will automatically use the latest version, but if you're viewing the page of a specific version then maybe it would make sense to include the version in the command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I didn't consider version pages. I added a commit that adds the version only on version detail pages. The regular crate detail page still shows the versionless variant. That makes the most sense, I think?

Copy link

Choose a reason for hiding this comment

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

Good point! I didn't consider version pages. I added a commit that adds the version only on version detail pages. The regular crate detail page still shows the versionless variant. That makes the most sense, I think?

This is exactly the solution I was imagining when I was considering this idea as cargo-add was being merged into cargo.

Thanks for taking this on! I originally held off on getting this done as I wanted more time for versions of cargo with add to proliferate but lost track of coming back to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a good reason to delay it. I was surprised, looking up, that we got it in Rust 1.62.0 in June. Almost half a year ago! Doesn't feel that way. 😅

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

awesome! thanks again! :)

@Turbo87 Turbo87 merged commit 0358ca9 into rust-lang:master Jan 6, 2023
@stephank stephank deleted the feat-cargo-add branch January 6, 2023 20:13
@nrxus
Copy link

nrxus commented Jan 7, 2023

Is there a way for a crate to specify that it's meant to be a dev dependency only? I am an author for a crate that explicitly only wants to be added as a dev dependency because it modifies a lot of code at compile time to make structs mockable. While just adding the dependency alone won't hurt anything my README explicitly states to only add it as a dev dependency to make it fool proof that nothing bad can happen outside of tests. While I understand the motivation for this change I think it unintentionally implies that all crates are meant to be installed the same which is not the case which could cause issues for crates that intentionally want to only be for tests or during build.

@lnicola
Copy link
Member

lnicola commented Jan 7, 2023

@nrxus Sorry for the previous cargo add --dev comment, I submitted it too quickly.

That said, I've found that even crates usually meant to be used as dev dependencies can be useful in other contexts, like code generator tools, perhaps. I suppose you could check for cfg(test) somewhere in your generated code, though that's indeed not ideal.

If you only want to discourage (and not prohibit) your create from being used as a normal dependency, a way to specify that (like a flag in the manifest) would be nice.

@Turbo87
Copy link
Member

Turbo87 commented Jan 7, 2023

@nrxus I don't think there are currently any flags implemented to indicate that. I'm open to the suggestion though. Do you have any thoughts on how we could implement it?

@nrxus
Copy link

nrxus commented Jan 7, 2023

The most flexible thing would be to an extra field in Cargo.toml that allows you to override the install command shown in crates.io:

[package]
name = "faux"
install-command = "cargo add --dev"

@realchonk
Copy link

The most flexible thing would be to an extra field in Cargo.toml that allows you to override the install command shown in crates.io:

[package]
name = "faux"
install-command = "cargo add --dev"

@nrxus I don't think that this is ideal, because someone malicious could do something like that:

install-command = "rm -rf *"

I would opt for something like this:

add-as = "dev" # or "build"

It's a little uglier and less flexible, but safe. Otherwise you'd have to sanitize/check the install-command for every crate.

@nrxus
Copy link

nrxus commented Jan 7, 2023

That makes sense to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants