Limit cargo add feature print #12662

Angelin01 · 2023-09-13T02:06:02Z

Readyness checklist

Test validating activated features under limit, total over limit
Test validating activated features over limit
At most 49 features are displayed
"X more features" messages are displayed
~~[ ] The "X more features" messages should be hyperlinks~~

What does this PR try to resolve?

Limits the amount of printed features to 49. After that, it simply prints "... X more activated features" and "... Y more deactivated features".

How should we test and review this PR?

Two new tests were added to validate the functionality, see tests/testsuite/cargo_add/features_too_many_activated
and tests/testsuite/cargo_add/features_too_many_few_activated.

The first commit shows the output before this PR, the second with the modifications.

To test it live, run cargo add with a crate with a ton of features, such as web-sys

A few commands and sample outputs:

Adding web-sys with no features activated

Command:

cargo add --dry-run web-sys

Output:

      Updating crates.io index
      Adding web-sys v0.3.64 to dependencies.
             Features:
             - AbortController
             - AbortSignal
             - AddEventListenerOptions
             - AesCbcParams
             - AesCtrParams
             - AesDerivedKeyParams
             - AesGcmParams
             - AesKeyAlgorithm
             - AesKeyGenParams
             - Algorithm
             - AlignSetting
             - AllowedBluetoothDevice
             - AllowedUsbDevice
             - AlphaOption
             - AnalyserNode
             - AnalyserOptions
             - AngleInstancedArrays
             - Animation
             - AnimationEffect
             - AnimationEvent
             - AnimationEventInit
             - AnimationPlayState
             - AnimationPlaybackEvent
             - AnimationPlaybackEventInit
             - AnimationPropertyDetails
             - AnimationPropertyValueDetails
             - AnimationTimeline
             - AssignedNodesOptions
             - AttestationConveyancePreference
             - Attr
             - AttributeNameValue
             - AudioBuffer
             - AudioBufferOptions
             - AudioBufferSourceNode
             - AudioBufferSourceOptions
             - AudioConfiguration
             - AudioContext
             - AudioContextOptions
             - AudioContextState
             - AudioData
             - AudioDataCopyToOptions
             - AudioDataInit
             - AudioDecoder
             - AudioDecoderConfig
             - AudioDecoderInit
             - AudioDecoderSupport
             - AudioDestinationNode
             - AudioEncoder
             - AudioEncoderConfig
             - AudioEncoderInit
             ... 1516 more deactivated features
warning: aborting add due to dry run

Adding web-sys with only some features activated

Command: ```shell cargo add --dry-run web-sys -F AbortController -F AbortSignal -F AudioEncoder ```

Output:

    Updating crates.io index
      Adding web-sys v0.3.64 to dependencies.
             Features:
             + AbortController
             + AbortSignal
             + AudioEncoder
             + EventTarget
             - AddEventListenerOptions
             - AesCbcParams
             - AesCtrParams
             - AesDerivedKeyParams
             - AesGcmParams
             - AesKeyAlgorithm
             - AesKeyGenParams
             - Algorithm
             - AlignSetting
             - AllowedBluetoothDevice
             - AllowedUsbDevice
             - AlphaOption
             - AnalyserNode
             - AnalyserOptions
             - AngleInstancedArrays
             - Animation
             - AnimationEffect
             - AnimationEvent
             - AnimationEventInit
             - AnimationPlayState
             - AnimationPlaybackEvent
             - AnimationPlaybackEventInit
             - AnimationPropertyDetails
             - AnimationPropertyValueDetails
             - AnimationTimeline
             - AssignedNodesOptions
             - AttestationConveyancePreference
             - Attr
             - AttributeNameValue
             - AudioBuffer
             - AudioBufferOptions
             - AudioBufferSourceNode
             - AudioBufferSourceOptions
             - AudioConfiguration
             - AudioContext
             - AudioContextOptions
             - AudioContextState
             - AudioData
             - AudioDataCopyToOptions
             - AudioDataInit
             - AudioDecoder
             - AudioDecoderConfig
             - AudioDecoderInit
             - AudioDecoderSupport
             - AudioDestinationNode
             - AudioEncoderConfig
             ... 1516 more deactivated features

Adding web-sys with only more than 50 features activated

Command: ```shell cargo add --dry-run web-sys -F AbortController -F AbortSignal -F AddEventListenerOptions -F AesCbcParams -F AesCtrParams -F AesDerivedKeyParams -F AesGcmParams -F AesKeyAlgorithm -F AesKeyGenParams -F Algorithm -F AlignSetting -F AllowedBluetoothDevice -F AllowedUsbDevice -F AlphaOption -F AnalyserNode -F AnalyserOptions -F AngleInstancedArrays -F Animation -F AnimationEffect -F AnimationEvent -F AnimationEventInit -F AnimationPlayState -F AnimationPlaybackEvent -F AnimationPlaybackEventInit -F AnimationPropertyDetails -F AnimationPropertyValueDetails -F AnimationTimeline -F AssignedNodesOptions -F AttestationConveyancePreference -F Attr -F AttributeNameValue -F AudioBuffer -F AudioBufferOptions -F AudioBufferSourceNode -F AudioBufferSourceOptions -F AudioConfiguration -F AudioContext -F AudioContextOptions -F AudioContextState -F AudioData -F AudioDataCopyToOptions -F AudioDataInit -F AudioDecoder -F AudioDecoderConfig -F AudioDecoderInit -F AudioDecoderSupport -F AudioDestinationNode -F AudioEncoder -F AudioEncoderConfig -F AudioEncoderInit ```

Output:

    Updating crates.io index
      Adding web-sys v0.3.64 to dependencies.
             Features:
             + AbortController
             + AbortSignal
             + AddEventListenerOptions
             + AesCbcParams
             + AesCtrParams
             + AesDerivedKeyParams
             + AesGcmParams
             + AesKeyAlgorithm
             + AesKeyGenParams
             + Algorithm
             + AlignSetting
             + AllowedBluetoothDevice
             + AllowedUsbDevice
             + AlphaOption
             + AnalyserNode
             + AnalyserOptions
             + AngleInstancedArrays
             + Animation
             + AnimationEffect
             + AnimationEvent
             + AnimationEventInit
             + AnimationPlayState
             + AnimationPlaybackEvent
             + AnimationPlaybackEventInit
             + AnimationPropertyDetails
             + AnimationPropertyValueDetails
             + AnimationTimeline
             + AssignedNodesOptions
             + AttestationConveyancePreference
             + Attr
             + AttributeNameValue
             + AudioBuffer
             + AudioBufferOptions
             + AudioBufferSourceNode
             + AudioBufferSourceOptions
             + AudioConfiguration
             + AudioContext
             + AudioContextOptions
             + AudioContextState
             + AudioData
             + AudioDataCopyToOptions
             + AudioDataInit
             + AudioDecoder
             + AudioDecoderConfig
             + AudioDecoderInit
             + AudioDecoderSupport
             + AudioDestinationNode
             + AudioEncoder
             + AudioEncoderConfig
             + AudioEncoderInit
             ... 6 more activated features
             ... 1510 more deactivated features
warning: aborting add due to dry run

Additional information

I'd appreciate code style suggestions, if anything pops out. Even though I used to work with C/C++ a lot in older times, these days I spent way too much time with Bash, Python, Java and Jenkins' Groovy and I fear I let my mannerisms leak into my Rust code sometimes.

rustbot · 2023-09-13T02:06:07Z

r? @ehuss

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

epage · 2023-09-13T02:10:25Z

The "X more features" messages should be hyperlinks

Let's skip this for the MVP because that gets messy and I want to wait until we switch to anstream which will make it less bad

epage · 2023-09-13T02:11:52Z

Could you update this so the first commit adds a test showing the original behavior and then the commit with the condensed view will be the following commit and will then show how the view changes?

Test cases I expect

Activated features under limit, total features over limit
Just activated features over limit

src/cargo/ops/cargo_add/mod.rs

bors · 2023-09-13T03:30:31Z

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

Angelin01 · 2023-09-13T15:05:45Z

I've added the tests as required and rebased with the changes from #12655.

^{Would you believe my blind self actually missed the entire tests folder at first? I was actually grepping for #[cfg(test)] thinking I must've been insane.}

As a side note, the test code for both tests is almost identical, but I noticed that that's also the case for the rest of the cargo_add tests. Would you prefer I try to do some de-duplication or are we good to go?

weihanglo

Would you prefer I try to do some de-duplication or are we good to go?

The two newly added test cases look good to me :)

src/cargo/ops/cargo_add/mod.rs

weihanglo

Thanks for the update. I believe this meets what epage expects pretty much. I am going to merge it :)

weihanglo · 2023-09-14T13:19:10Z

@bors r+

bors · 2023-09-14T13:19:12Z

📌 Commit 9928689 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-09-14T13:19:19Z

⌛ Testing commit 9928689 with merge 5c84ce0...

Angelin01 · 2023-09-14T13:22:46Z

Thank you @weihanglo and @epage . I feel like these things are not said enough, but so far these little pull requests have been the simplest, fastest, most straight forward and nice open source contributions I've ever been able to do. My applause to the maintainers, you're doing a great job!

bors · 2023-09-14T14:07:31Z

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

epage · 2023-09-14T16:50:48Z

@Angelin01 I think there was a miscommunication that led to this being merged. This started as a draft, so I was unsure where it was at compared to #12617, so I didn't say anything about it yet but this does not match the last UX proposed in the issue and I would have expected it to match or for more discussion to have happened before moving onto the PR if there was a disagreement about it (which is fully valid to do).

Angelin01 · 2023-09-14T16:57:04Z

@Angelin01 I think there was a miscommunication that led to this being merged. This started as a draft, so I was unsure where it was at compared to #12617, so I didn't say anything about it yet but this does not match the last UX proposed in the issue and I would have expected it to match or for more discussion to have happened before moving onto the PR if there was a disagreement about it (which is fully valid to do).

@epage I simply went with @weihanglo review, I assumed things were ok! At least, I thought the format was settled, as I had provided sample outputs in the PR description and tests, but we obviously jumped the gun!
Feel free to revert the PR and reopen the issue and we'll figure out the best UX!

epage · 2023-09-14T20:10:02Z

At least, I thought the format was settled, as I had provided sample outputs in the PR description and tests, but we obviously jumped the gun!

As I said, this should have been brought up in the Issue before creating the PR. In some cases, the PR process discovers new information then it can be brought up then and that should be explicitly called out rather than hoping that the reviewer notices the discrepancy.

Feel free to revert the PR and reopen the issue and we'll figure out the best UX!

Open to just doing a follow up PR to go with the original design or do you consider this unresolved enough that we should undo it?

Angelin01 · 2023-09-14T20:22:33Z

Open to just doing a follow up PR to go with the original design or do you consider this unresolved enough that we should undo it?

@epage I can certainly do a follow up, most of the work here can be used. Feel free to open a new issue or reopen the issue and @ me, but I think I need clarifying on what the original design is, I'm still unsure what does not match what was expected in the issue (aside from the link, which we discarded because of the library migration).

weihanglo · 2023-09-15T03:35:38Z

@Angelin01 that was my fault. Sorry. I saw this comment and though you've agreed on something 😅.

Update cargo 11 commits in 2fc85d15a542bfb610aff7682073412cf635352f..d5336f813df39d476e61fc46daabb1446350660a 2023-09-09 01:49:46 +0000 to 2023-09-14 19:55:49 +0000 - fix: emit a warning for `credential-alias` shadowing (rust-lang/cargo#12671) - refactor: fix lint errors in preparation of `[lints]` table integration (rust-lang/cargo#12669) - Limit cargo add feature print (rust-lang/cargo#12662) - Clippy (rust-lang/cargo#12667) - Prerelease candidates error message (rust-lang/cargo#12659) - Fix typos: `informations` -> `information` (rust-lang/cargo#12666) - chore: update globset to 0.4.13 (rust-lang/cargo#12665) - refactor: Consolidate clap/shell styles (rust-lang/cargo#12655) - libgit2 fixed upstream (rust-lang/cargo#12657) - Index summary enum (rust-lang/cargo#12643) - feat(help): Add styling to help output (rust-lang/cargo#12578) r? ghost

rustbot assigned ehuss Sep 13, 2023

rustbot added Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2023

epage reviewed Sep 13, 2023

View reviewed changes

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved

Showcase output with too many features

3a40df8

Angelin01 marked this pull request as ready for review September 13, 2023 15:05

weihanglo reviewed Sep 14, 2023

View reviewed changes

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved

src/cargo/ops/cargo_add/mod.rs Outdated Show resolved Hide resolved

Limit features printed during cargo add

9928689

weihanglo approved these changes Sep 14, 2023

View reviewed changes

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 14, 2023

bors merged commit 5c84ce0 into rust-lang:master Sep 14, 2023
21 checks passed

Angelin01 deleted the limit-cargo-add-feature-print branch September 14, 2023 14:08

Angelin01 restored the limit-cargo-add-feature-print branch September 14, 2023 17:00

weihanglo mentioned this pull request Sep 15, 2023

Update cargo rust-lang/rust#115878

Merged

Angelin01 mentioned this pull request Sep 19, 2023

Cargo add displays either feature list or summarized count #12702

Merged

ehuss added this to the 1.74.0 milestone Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit cargo add feature print #12662

Limit cargo add feature print #12662

Angelin01 commented Sep 13, 2023 •

edited

Loading

rustbot commented Sep 13, 2023

epage commented Sep 13, 2023

epage commented Sep 13, 2023

bors commented Sep 13, 2023

Angelin01 commented Sep 13, 2023

weihanglo left a comment

weihanglo left a comment

weihanglo commented Sep 14, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

Angelin01 commented Sep 14, 2023

bors commented Sep 14, 2023

epage commented Sep 14, 2023

Angelin01 commented Sep 14, 2023 •

edited

Loading

epage commented Sep 14, 2023

Angelin01 commented Sep 14, 2023

weihanglo commented Sep 15, 2023

Limit cargo add feature print #12662

Limit cargo add feature print #12662

Conversation

Angelin01 commented Sep 13, 2023 • edited Loading

Readyness checklist

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Sep 13, 2023

epage commented Sep 13, 2023

epage commented Sep 13, 2023

bors commented Sep 13, 2023

Angelin01 commented Sep 13, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Sep 14, 2023

bors commented Sep 14, 2023

bors commented Sep 14, 2023

Angelin01 commented Sep 14, 2023

bors commented Sep 14, 2023

epage commented Sep 14, 2023

Angelin01 commented Sep 14, 2023 • edited Loading

epage commented Sep 14, 2023

Angelin01 commented Sep 14, 2023

weihanglo commented Sep 15, 2023

Angelin01 commented Sep 13, 2023 •

edited

Loading

Angelin01 commented Sep 14, 2023 •

edited

Loading