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

Support cargo owner add #11879

Closed
wants to merge 42 commits into from
Closed

Support cargo owner add #11879

wants to merge 42 commits into from

Conversation

heisen-li
Copy link
Contributor

@heisen-li heisen-li commented Mar 23, 2023

What does this PR try to resolve?

User confusion over how to invoke it, including

  • cargo owner doing nothing, rather than erroring and telling the user how to use it
  • Users assuming the interface uses subcommands, rather than action flags

This is done by deprecating the old interface and replacing it with subcommands.

To do this

  • cargo owner will now be synonymous with cargo owner --help except a bad error code will be returned
  • cargo owner --help includes a usage for each subcommand (Display help for all subcommands at the same time clap-rs/clap#1334 could do this for us)
  • --add, --list, and --remove are deprecated in favor of add, list, and remove
    • Using a flag with a subcommand is a hard error
    • The old flags are hidden in --help

This new interface loses the ability to do multiple operations at once but that seems like a premature optimization.

Additional information

Breaking change: cargo owner used to be a no-op (no actions specified) but will now be a hard error. This change is being made assuming the the use case for running cargo owner without any actions in a programmatic setting is small enough that this is acceptable

Fixes #4352

* upstream/master:
  docs: fix typos in `cargo_compile/mod.rs`
  docs(contrib): Replace architecture with redirects
  docs(contrub): Remove unused file
  docs(contrib): Move some lower resolver details from ops to core
  docs(contrib): Move higher level resolver docs into doc comments
  docs(contrib): Pull impl info out of architecture
@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2023

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-interacts-with-crates.io Area: interaction with registries Command-owner S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2023
@heisen-li heisen-li changed the title [WIP] Support cargo owner add [WIP] Support cargo owner add Mar 23, 2023
heisen-li and others added 3 commits April 2, 2023 16:39
* upstream/owner: (21 commits)
  Don't place side-effect expressions in assert! macros.
  Add `_MS` suffix to retry constants.
  Add some more docs and comments to `SleepTracker`.
  Add delays to network retries.
  Allow RegistryBuilder responder URLs to be a String
  Split the `cargo::util::network` module into submodules
  Add a note to `cargo logout` that it does not revoke the token.
  Sync external-tools JSON docs.
  Disable test_profile test on windows-gnu
  Drop derive feature from serde in cargo-platform
  a{n =>} benchmark target
  documented working directory behaviour of cargo-test, cargo-bench and cargo-run commands
  chore: Upgrade to clap v4.2
  docs(contrib): Link to office hours doc
  doc(contrib): missing quotation mark
  Update changelog for 1.68.2
  Add the old github keys as revoked
  Update proptest
  Added new GitHub RSA Host Key
  doc: Fix registries.name.index for sparse
  ...
@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Apr 2, 2023
@heisen-li heisen-li changed the title [WIP] Support cargo owner add Support cargo owner add Apr 2, 2023
@heisen-li
Copy link
Contributor Author

heisen-li commented Apr 2, 2023

So far, it seems that no one has pointed out any mistakes in the direction that I am taking with my current actions. Therefore, I completed the remaining tasks today.

The main modifications are:

1.Management of crate owners will be conducted using sub-commands instead of mandatory parameters, for example: cargo owner add/remove/list;
2.Executing cargo owner or cargo owner add/remove will prompt errors or return help information, which I believe will be helpful for users;
3.The previous functionality of using cargo owner --add someone1 --add someone2 has been retained. Now, you can use cargo owner add someone1, someone2 instead.
4.Test cases and corresponding documentation in cargo book have been modified.

Very welcome your suggestion. Thank you for your review.

heisen-li and others added 5 commits November 20, 2023 20:06
* 'owner' of https://github.com/heisen-li/cargo: (360 commits)
  refactor(toml): Better abstract inheritance details
  Deduplicate the similar dependency.
  Update contrib docs on rustfix issue tracking.
  Add rustfix lib to autolabel triggers.
  Add a general introduction to the rustfix library docs.
  Ignore rustfix for semver-checks since it doesn't exist on the beta branch.
  typo: rusc -> rustc
  Handle $message_type in JSON diagnostics
  Fix --check-cfg invocation with zero features
  chore: bump cargo-credential-* crates as e58b84d broke stuff
  Remove copyright headers in tests.
  Fix tests to run on stable.
  Fix clippy warnings.
  contrib docs: Update now that credential crates are published.
  Add more resources to the contrib docs.
  Add rustfix to publish.
  Add optional flag to manifest for dependencies
  Add features to the default Cargo.toml file
  refactor(toml): Move accessor to be part of schema API
  fix(toml): Prevent workspace=false in API
  ...
@rustbot rustbot added the A-completions Area: shell completions label Nov 25, 2023
src/etc/cargo.bashcomp.sh Outdated Show resolved Hide resolved
src/etc/_cargo Outdated
Comment on lines 220 to 225
'(add)'{add}'[specify name of a user or team to invite as an owner]:name' \
'--index=[specify registry index]:index' \
'(-l --list)'{-l,--list}'[list owners of a crate]' \
'(list)'{list}'[list owners of a crate]' \
'(-r --remove)'{-r,--remove}'[specify name of a user or team to remove as an owner]:name' \
'(remove)'{remove}'[specify name of a user or team to remove as an owner]:name' \
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect these will let us complete the subcommand names but not the arguments underneath? Or if they do, it'll report all the arguments underneath?

Unfortunately, it looks like zsh completions are missing cargo report future-incompat completions for us to copy from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Command completion for zsh is a bit difficult for me and I'm learning how to accomplish 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.

I've tried to finish. In addition, whether cargo report future-incompat should be corrected in the new issue.

Comment on lines +112 to +118
_ => (
args.get_many::<String>("add")
.map(|xs| xs.cloned().collect::<Vec<String>>()),
args.get_many::<String>("remove")
.map(|xs| xs.cloned().collect()),
args.flag("list"),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is meant to operate for None. We should separate out Some(_) and panic for it as we should only get the subcommands back we defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no subcommands or any parameters(like:--add,..), an error message will be displayed instead of panic.

https://github.com/rust-lang/cargo/pull/11879/files#diff-981bc8fd9f47eff85de2c539a5be58b0002e2f060064a090218060c5ae3e5722R114-R122

@bors
Copy link
Contributor

bors commented Dec 6, 2023

☔ The latest upstream changes (presumably #13117) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jan 2, 2024

☔ The latest upstream changes (presumably #13227) made this pull request unmergeable. Please resolve the merge conflicts.

@heisen-li
Copy link
Contributor Author

@epage I'm sorry that this PR is taking too much of your effort, but I might have to ask, is there anything I need to revise further at the moment?

If the PR does not meet the community's acceptance rules, I can choose to close it. Please let me know if you need to modify it. All for the better cargo.

@bors
Copy link
Contributor

bors commented Feb 20, 2024

☔ The latest upstream changes (presumably #13409) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
Copy link
Member

I am going to close this as it has been stable for a while. The test infra in Cargo changed a lot during the period, so it is unavoidable to rework on it. Also, there were too many comments in this PR, making it hard to track the actual progress (Please stop folding everything GitHub!)

Feel free to discuss the design in #4352 and implementation details on Zulip t-cargo. Thank you!

@weihanglo weihanglo closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-cli-help Area: built-in command-line help A-completions Area: shell completions A-documenting-cargo-itself Area: Cargo's documentation A-interacts-with-crates.io Area: interaction with registries Command-owner S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support cargo owner add
5 participants