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

fix(add): Clarify which version the features are added for #11075

Merged
merged 11 commits into from
Sep 13, 2022
Merged

fix(add): Clarify which version the features are added for #11075

merged 11 commits into from
Sep 13, 2022

Conversation

epage
Copy link
Contributor

@epage epage commented Sep 12, 2022

What does this PR try to resolve?

This gives a hint to users that we might not be showing the feature list
for the latest version but the earliest version.

Also when using a workspace dependency, this is a good reminder of what the version requirement is that was selected. That could also be useful for reused dependencies but didn't want to bother with the relevant plumbing for that.

ie we are going from

$ cargo add chrono@0.4
    Updating crates.io index
      Adding chrono v0.4 to dependencies.
             Features:
             - rustc-serialize
             - serde

to

$ cargo add chrono@0.4
    Updating crates.io index
      Adding chrono v0.4 to dependencies.
             Features as of v0.4.2:
             - rustc-serialize
             - serde

How should we test and review this PR?

I'd recommend looking at this commit-by-commit. This is broken up into several refactors leading up the main change. The refactors are focused on pulling UI logic out of dependency editing so we can more easily evolve the UI without breaking the editing API. I then tweaked the behavior in the final commit to be less redundant / noisy.

The existing tests are used to demonstrate this is working.

Additional information

I'm also mixed on whether the meta version should show up.

Fixes #11073

@epage
style(add): Clean up Context import
ddd44c8
@epage
refactor(add): Decouple available features from inheritance
74e1a70
@epage
refactor(add): Consolidate looking up available features
2da6c04 
This optimization doesn't really make a difference and it makes it
harder to separate UI / edit concerns.
@epage
refactor(add): Pull UI concerns out of edit logic
2715b56
@epage
refactor(add): Make it easier to add more summary info
391281b
@epage
fix(add): Clarify which version the features are added for
73906ae 
This gives a hint to users that we might not be showing the feature list
for the latest version but the earliest version.

Also when using a workspace dependency or re-using an existing
dependency, this is a good reminder of what the version requirement is
that was selected.

However, when the user or add (the common case) selected a full
precision requirement, this is redundant.

I'm also mixed on whether the meta version should show up.

Fixes rust-lang#11073
@epage
fix(add): Limit 'Features as of vX.Y.Z' to when relevant
7ce2b7d 
This will only show the messaeg if we didn't already show a version req
with full precision specified ... mostly.  We'll also skip it if its a
local or git dependency but we never show the version in those cases
because it doesn't matter.

The `precise_version` logic came from cargo-upgrade.
@epage
fix(add): Remove redundant version in build metadata cases
be257b8
@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 12, 2022
weihanglo
weihanglo reviewed Sep 13, 2022
View reviewed changes
src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_add/mod.rs Show resolved Hide resolved
src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved
weihanglo
weihanglo approved these changes Sep 13, 2022
View reviewed changes
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.

I am good with this. Thank you!

@Muscraft do you have any other suggestion?

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 13, 2022

📌 Commit 5a6cfc9 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 13, 2022
@bors
Copy link
Contributor

bors commented Sep 13, 2022

⌛ Testing commit 5a6cfc9 with merge c633b27...

@bors
Copy link
Contributor

bors commented Sep 13, 2022

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

@bors bors merged commit c633b27 into rust-lang:master Sep 13, 2022
@bors bors mentioned this pull request Sep 13, 2022
Merged
weihanglo added a commit to weihanglo/rust that referenced this pull request Sep 13, 2022
@weihanglo
Update cargo
3dfdfdf 
10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63
2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
@weihanglo weihanglo mentioned this pull request Sep 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 13, 2022
@bors
Auto merge of rust-lang#101776 - weihanglo:update-cargo, r=ehuss
88a1922 
Update cargo

10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63 2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
@epage epage deleted the feat branch September 16, 2022 16:33
@ehuss ehuss added this to the 1.65.0 milestone Sep 21, 2022
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
@bors
Auto merge of #101776 - weihanglo:update-cargo, r=ehuss
98e1e6b 
Update cargo

10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63 2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #101776 - weihanglo:update-cargo, r=ehuss
2d2ab83 
Update cargo

10 commits in 646e9a0b9ea8354cc409d05f10e8dc752c5de78e..082503982ea0fb7a8fd72210427d43a2e2128a63 2022-09-02 14:29:28 +0000 to 2022-09-13 17:49:38 +0000
- Take priority into account within the pending queue (rust-lang/cargo#11032)
- fix(add): Clarify which version the features are added for (rust-lang/cargo#11075)
- doc: clarify config-relative paths for `--config <path>` (rust-lang/cargo#11079)
- Do not add home bin path to PATH if it's already there (rust-lang/cargo#11023)
- Don't use `for` loop on an `Option` (rust-lang/cargo#11081)
- Remove dead code (rust-lang/cargo#11080)
- Change progress indicator for sparse registries (rust-lang/cargo#11068)
- chore(ci): Ensure intradoc links are valid (rust-lang/cargo#11055)
- Cache index files based on contents hash (rust-lang/cargo#11044)
- fix: specifies the max length for crate name (rust-lang/cargo#11051)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Muscraft Muscraft Muscraft approved these changes

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
Command-add S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

cargo add package@version shows features from oldest instead of current version
6 participants
@epage @rust-highfive @weihanglo @bors @Muscraft @ehuss