-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(toml): Allow adding/removing from cargo scripts #14857
Conversation
2a5fdea
to
6fa341d
Compare
// Ok, this is a shebang but if the next non-whitespace token is `[`, | ||
// then it may be valid Rust code, so consider it Rust code. | ||
if rest.trim_start().starts_with('[') { | ||
return Ok(source); | ||
} |
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.
Just a comment: This isn't quite the same behavior, as the compiler allows comments before the opening [
. I realize parsing that would be difficult, and that particular scenario is a bit silly.
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 is existing code, just moved. Ok if we put this in the tracking issue or an issue and track separately?
if !manifest.ends_with("\n") { | ||
manifest.push_str("\n"); | ||
} | ||
let fence = "---\n"; |
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.
Just a comment: These look like it will cause mixed line endings on a CRLF file. However, that looks like it is an existing problem with cargo add
in general.
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.
As this is another manifestation of an existing issue, are we ok with deferring this out of this PR?
This will make it easier to add cargo script support
a6a3dfe
to
26844a3
Compare
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!
What does this PR try to resolve?
This adds support for cargo script for more cargo commands and is a part of #12207
Unfortunately, there is a double-warning for unspecified editions for
cargo remove
. Rather than addressing that here, I'll be noting it in the tracking issue.How should we test and review this PR?
Additional information