Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

fix: Improve feature and dependency rendering #140

Merged
merged 6 commits into from
Apr 9, 2024

Conversation

epage
Copy link
Collaborator

@epage epage commented Apr 1, 2024

This matches a proposal in rust-lang/cargo#10681 for how to render features in cargo-add.

A bit unsure on the dependency rendering.

Fixes #26

@epage epage requested a review from Rustin170506 April 1, 2024 20:17
Copy link
Owner

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Copy link
Owner

Choose a reason for hiding this comment

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

I am curious what if we also keep the color here? For instance:

  1. Enabled by user: + and colored
  2. Enabled: colored
  3. Disable: nothing

Because it seems hard to tell from the picture, which features are included in the features that are turned on by default(or by users).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was confused what you are talking about until I realized that we needed #142 to properly render the new style of output.

Does the new output change your thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good to me! Thank you.

@Rustin170506
Copy link
Owner

A bit unsure on the dependency rendering.

I am hesitant about whether we should use the symbol "+" or not. This might confuse the users as "+" usually indicates addition. I am uncertain if this will cause any confusion.

@epage
Copy link
Collaborator Author

epage commented Apr 5, 2024

I am hesitant about whether we should use the symbol "+" or not. This might confuse the users as "+" usually indicates addition. I am uncertain if this will cause any confusion.

I'd like to present the three states

  • required deps
  • activated optional deps
  • inactive optional deps

Any other thoughts for doing so? Or should we give up and only do required vs optional?

Copy link
Owner

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks! 👍

@Rustin170506
Copy link
Owner

Any other thoughts for doing so? Or should we give up and only do required vs optional?

I don't have a strong opinion on this. We can release it to see what others think.

@Rustin170506
Copy link
Owner

@epage Do you think this PR is ready to go? I am preparing for a new release.

@epage
Copy link
Collaborator Author

epage commented Apr 9, 2024

Yes, it is

@Rustin170506 Rustin170506 merged commit 863447c into Rustin170506:main Apr 9, 2024
9 checks passed
@Rustin170506
Copy link
Owner

Thanks for working on this again! 🤟 🖤

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How should we render features?
2 participants