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.

Sign up for GitHub

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

Jump to bottom

Rename --all to --workspace? #3911

Closed
lnicola opened this issue Nov 7, 2019 · 6 comments
Closed

Rename --all to --workspace? #3911

lnicola opened this issue Nov 7, 2019 · 6 comments
Labels
1x-backport:completed

Comments

@lnicola
Copy link
Member

lnicola commented Nov 7, 2019

In rust-lang/cargo#7241, cargo --all got deprecated in favor of --workspace. There's less chance of confusion in cargo fmt because there is no `--all-targets, but perhaps it's a good idea to support both for consistency.
@calebcartwright
Copy link
Member

calebcartwright commented Nov 7, 2019 •
edited
Loading

Thanks @lnicola, I wasn't aware of the change to --workspace in cargo! There's certainly a case to be made for consistency, and this would be as good a time as any as the current focus is on rustfmt 2.0 and there's other arg/option changes. There are, however, certain behaviors associated with cargo fmt's args that IMO are worth considering.

For example, when running cargo fmt in the root directory of a workspace, cargo fmt already formats all of the workspace members. Running cargo fmt --all formats the workspace plus any local/path based dependencies of the packages in that workspace, regardless of whether those local deps are explicit workspace members.

If the format strategy currently used with --all were renamed to --workspace, would it be confusing/surprising for users that their entire workspace gets formatted when they run cargo fmt even without the --workspace arg? Similarly, would it be confusing/surprising for users when running cargo fmt --workspace that those local/deps outside the workspace were formatted?

@lnicola
Copy link
Member Author

lnicola commented Nov 7, 2019

Sorry, I wasn't aware of what --all did. I went by the help message:

--all Format all packages (only usable in workspaces)

So I hope you understand my confusion. If it really behaves like you described, I don't think it's worth making any change here.

@lnicola lnicola closed this as completed Nov 7, 2019
@calebcartwright
Copy link
Member

No worries! It may still be worth considering a rename, but I just wanted to note some of the other aspects that would need to be weighed against the consistency with cargo's --workspace

I can see how that message could be confusing, especially that suffix. I believe --all can technically be used today even outside an explicit workspace so we should probably update that message either way. Perhaps something like the below would be more clear?

--all Format all packages and their local path-based dependencies

@topecongiro
Copy link
Contributor

@calebcartwright Seems good to me 🙌

@lnicola lnicola mentioned this issue Nov 8, 2019
Merged
@lnicola
Copy link
Member Author

lnicola commented Nov 8, 2019

Filed #3914.

dkim added a commit to dkim/chip8 that referenced this issue Jan 12, 2020
@dkim
ci: replace --all in cargo clippy/test with --workspace
ffc3491 
The `--all` flag in `cargo check/clippy/test` has been deprecated in
favor of `--workspace`: rust-lang/cargo#7241.

`--all` in `cargo fmt` is retained because it has a little different
meaning. From rust-lang/rustfmt#3911:

> For example, when running `cargo fmt` in the root directory of a
> workspace, `cargo fmt` already formats all of the workspace members.
> Running `cargo fmt --all` formats the workspace *plus* any local/path
> based dependencies of the packages in that workspace, regardless of
> whether those local deps are explicit workspace members.
dkim added a commit to dkim/chip8 that referenced this issue Jan 13, 2020
@dkim
ci: replace --all in cargo clippy/test with --workspace
f1636d5 
The `--all` flag in `cargo check/clippy/test` has been deprecated in
favor of `--workspace`: rust-lang/cargo#7241.

`--all` in `cargo fmt` is retained because it has a little different
meaning. From rust-lang/rustfmt#3911:

> For example, when running `cargo fmt` in the root directory of a
> workspace, `cargo fmt` already formats all of the workspace members.
> Running `cargo fmt --all` formats the workspace *plus* any local/path
> based dependencies of the packages in that workspace, regardless of
> whether those local deps are explicit workspace members.
@nlordell nlordell mentioned this issue Jan 16, 2020
Merged
nlordell added a commit to cowprotocol/ethcontract-rs that referenced this issue Jan 16, 2020
@nlordell
[Easy] Update Cargo commands to use --workspace instead of --all (#…
87895d9 
…128)

Just update the cargo commands since `--all` is deprecated. From the help:
```
$ cargo build --help
# ...
        --all                        Alias for --workspace (deprecated)
        --workspace                  Build all packages in the workspace
# ...
```

Note that `cargo fmt` does not support the `--workspace` flag and `--all` actually has different semantics to `--workspace` for other cargo commands. Currently, `cargo fmt` already formats all packages in the workspace, the `--all` flag additionally formats packages that are imported via path and are outside of the workspace (see discussion [here](rust-lang/rustfmt#3911)). So, for this PR we also remove the `--all` flag as it superfluous.

### Test plan

CI

### Commit History

* update --all to --workspace

* --all not needed for fmt
@calebcartwright calebcartwright mentioned this issue Jun 16, 2020
Closed
@Blisto91 Blisto91 mentioned this issue Aug 20, 2020
Merged
8 tasks
@Nemo157 Nemo157 mentioned this issue Aug 24, 2020
Merged
@calebcartwright calebcartwright mentioned this issue Sep 27, 2020
Closed
@matklad matklad mentioned this issue Feb 25, 2021
Closed
@jplatte jplatte mentioned this issue Apr 12, 2021
Closed
@calebcartwright calebcartwright added the 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release label Sep 12, 2021
@calebcartwright calebcartwright mentioned this issue Sep 29, 2021
Closed
@karyon
Copy link
Contributor

karyon commented Oct 25, 2021

Backport done in #4989

@karyon karyon added 1x-backport:completed and removed 1x-backport:pending Fixed/resolved in source but not yet backported to a 1x branch and release labels Oct 25, 2021
jplatte added a commit to jplatte/gloo that referenced this issue Oct 25, 2021
@jplatte
Update contributing instructions
e9df5ba 
`--all` argument to `cargo fmt` is not necessary, see
rust-lang/rustfmt#3911
ranile pushed a commit to rustwasm/gloo that referenced this issue Oct 26, 2021
@jplatte
Re-export gloo-utils from gloo (#159)
53c6425 
* Re-export gloo-utils from gloo

* Update contributing instructions

`--all` argument to `cargo fmt` is not necessary, see
rust-lang/rustfmt#3911
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1x-backport:completed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@lnicola @karyon @calebcartwright @topecongiro