Skip to content

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 28, 2022

No description provided.

@rust-highfive
Copy link

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2022
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

The pull request looks great overall!

Personal take (not blockers):

  • I love the new way of handling trailing arguments. All stuff get more clear now!
  • A mix of text decorations and colors is a bit noisier to me.
    image

Thank you!

@epage
Copy link
Contributor Author

epage commented Sep 29, 2022

  • A mix of text decorations and colors is a bit noisier to me.
image

If it helps, --help is now bolded instead of green. Overall, point still stands and something for us to consider. Feel free to create an issue in clap

epage added a commit to epage/clap that referenced this pull request Sep 29, 2022
@epage epage force-pushed the clap4 branch 2 times, most recently from 8649be0 to 69ba69f Compare September 29, 2022 15:26
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

After reading through the clap v4 changelog, I don't think there is something would break Cargo's argument parsing behavior.

I totally trust and appreciate the great works by clap team and epage. Just not sure about others' stances of Cargo being a pioneer adopting new release of a crate. I am happy to merge it now. If anyone feels uncertain we can always revert it.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2022

📌 Commit 69ba69f has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2022
@bors
Copy link
Contributor

bors commented Sep 29, 2022

⌛ Testing commit 69ba69f with merge c39193a...

@bors
Copy link
Contributor

bors commented Sep 29, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing c39193a to master...

@bors bors merged commit c39193a into rust-lang:master Sep 29, 2022
@epage epage deleted the clap4 branch September 29, 2022 22:03
weihanglo added a commit to weihanglo/rust that referenced this pull request Oct 4, 2022
8 commits in f5fed93ba24607980647962c59863bbabb03ce14..0b84a35c2c7d70df4875a03eb19084b0e7a543ef
2022-09-27 12:03:57 +0000 to 2022-10-03 19:13:21 +0000

- Provide a better error message when mixing dep: with / (rust-lang/cargo#11172)
- Remove lingering unstable flag `-Zfeatures` (rust-lang/cargo#11168)
- Tweak wording (rust-lang/cargo#11164)
- Expose libgit2-sys/vendored feature as vendored-libgit2 (rust-lang/cargo#11162)
- refactor(cli): Upgrade to clap v4 (rust-lang/cargo#11159)
- Expose guide to adding a new edition as rustdoc (rust-lang/cargo#11157)
- Remove `multitarget` from -Zhelp (rust-lang/cargo#11158)
- Remove outdated comments (rust-lang/cargo#11155)
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2022
Update cargo

8 commits in f5fed93ba24607980647962c59863bbabb03ce14..0b84a35c2c7d70df4875a03eb19084b0e7a543ef 2022-09-27 12:03:57 +0000 to 2022-10-03 19:13:21 +0000

- Provide a better error message when mixing dep: with / (rust-lang/cargo#11172)
- Remove lingering unstable flag `-Zfeatures` (rust-lang/cargo#11168)
- Tweak wording (rust-lang/cargo#11164)
- Expose libgit2-sys/vendored feature as vendored-libgit2 (rust-lang/cargo#11162)
- refactor(cli): Upgrade to clap v4 (rust-lang/cargo#11159)
- Expose guide to adding a new edition as rustdoc (rust-lang/cargo#11157)
- Remove `multitarget` from -Zhelp (rust-lang/cargo#11158)
- Remove outdated comments (rust-lang/cargo#11155)
epage added a commit to epage/cargo that referenced this pull request Oct 7, 2022
When working on clap v4, it appeared that `last` and `trailing_var_arg`
are mutually exclusive, so I called that out in the debug asserts in
clap-rs/clap#4187.  Unfortunately, I didn't document my research on this
as my focus was elsewhere.

When updating cargo to use clap v4 in rust-lang#11159, I found `last` and
`trailing_var_arg` were used together.  I figured this was what I was
trying to catch with above and I went off of my intuitive sense of these
commands and assumed `trailing_var_arg` was intended, not knowing about
the `testname` positional that is mostly just a forward to `libtest`,
there for documentation purposes, except for a small optimization.

This restores us to behavior from 531ce13 which is what I originally
wrote the tests against.
epage added a commit to epage/cargo that referenced this pull request Oct 7, 2022
When working on clap v4, it appeared that `last` and `trailing_var_arg`
are mutually exclusive, so I called that out in the debug asserts in
clap-rs/clap#4187.  Unfortunately, I didn't document my research on this
as my focus was elsewhere.

When updating cargo to use clap v4 in rust-lang#11159, I found `last` and
`trailing_var_arg` were used together.  I figured this was what I was
trying to catch with above and I went off of my intuitive sense of these
commands and assumed `trailing_var_arg` was intended, not knowing about
the `testname` positional that is mostly just a forward to `libtest`,
there for documentation purposes, except for a small optimization.

So it looks like we just need the `last` call and not the
`trailing_var_arg` call.

This restores us to behavior from 531ce13 which is what I originally
wrote the tests against.
bors added a commit that referenced this pull request Oct 7, 2022
fix(test): Distinguish 'testname' from escaped arguments

When working on clap v4, it appeared that `last` and `trailing_var_arg`
are mutually exclusive, so I called that out in the debug asserts in
#4187.  Unfortunately, I didn't document my research on this
as my focus was elsewhere.

When updating cargo to use clap v4 in #11159, I found `last` and
`trailing_var_arg` were used together.  I figured this was what I was
trying to catch with above and I went off of my intuitive sense of these
commands and assumed `trailing_var_arg` was intended, not knowing about
the `testname` positional that is mostly just a forward to `libtest`,
there for documentation purposes, except for a small optimization.

So it looks like we just need the `last` call and not the
`trailing_var_arg` call.

This restores us to behavior from 531ce13 which is what I originally
wrote the tests against.

It looks like #11159 was merged after the last beta branch was made, so we shouldn't
need to cherry-pick this into any other release.

For reviewing this, I made the test updates in the first commit, showing the wrong behavior.  The following commit fixes the behavior and updates the tests to expected behavior.

Fixes #11191
@ehuss ehuss added this to the 1.66.0 milestone Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants