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

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 hi-rustin April 1, 2024 20:17
Copy link
Owner

@hi-rustin hi-rustin 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.

@hi-rustin
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

@hi-rustin hi-rustin 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! 👍

@hi-rustin
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.

@hi-rustin
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

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

Thanks for working on this again! 🤟 🖤

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

Successfully merging this pull request may close these issues.

How should we render features?
2 participants