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

Add DisableColoredHelp setting to improve flexibility #2956

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

pksunkara
Copy link
Member

@pksunkara pksunkara commented Oct 27, 2021

Until we have a modular help generator that can be configured and/or authored by the users themselves as part of #2914, we will provide the flexibility of turning off colored help messages but still wanting colored error messages.

This flexibility was available before #2845 and @dbrgn immediately noticed it and requested it back to which I agree. This was also suggested by Josh in here

@pksunkara pksunkara requested a review from a team October 29, 2021 11:16
@pksunkara
Copy link
Member Author

bors r=ldm0

@pksunkara
Copy link
Member Author

bors retry

Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

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

We specifically chose not to invert but to remove. I feel this needs further discussion

@epage
Copy link
Member

epage commented Oct 29, 2021

bors r-

@bors
Copy link
Contributor

bors bot commented Oct 29, 2021

Canceled.

@kbknapp
Copy link
Member

kbknapp commented Nov 3, 2021

For awareness to those not following; discussion is happening #2906

I'm going to catch up on that and give some feedback.

Comment on lines 576 to 574
#[cfg(feature = "color")]
DisableColoredHelp,
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't behind a feature flag before, should it be now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't a setting before. This is a new setting which is inverted.

Copy link
Member

Choose a reason for hiding this comment

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

That is just semantics, driven by our policy that a flag has to always represent the "true" state. Its us inverting an existing flag. The inverted form of this flag was not conditional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the previous one wasn't gated because the macro didn't support gates on the settings until recently when I added them.

Copy link
Member

Choose a reason for hiding this comment

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

So long as the feature gate is used in an additive manner (which it is here) I don't see a conflict.

@kbknapp
Copy link
Member

kbknapp commented Nov 16, 2021

"Final comment period"

I'm good with merging this in support of making the v3 transition easier. Understood it's not the best way and that this will be changed in future versions as we tackle some of the trickier aspects of reflection and such. But having said that @epage are there any other objections that should keep us from moving forward on this as is, that haven't been expressed? My goal is to get these PRs cleared out to keep marching towards the release.

@epage
Copy link
Member

epage commented Nov 16, 2021

The only remaining issue I had was for us to revert the feature gate on the setting. EnableColoredHelp didn't have the feature gate. Let's at least keep that for DisableColoredHelp for now. We can re-evaluate later if needed.

@pksunkara
Copy link
Member Author

Yeah, we have been having discussion on this. We had already decided to not have feature gates on reflection API but have them on builder API.

The issue here is that, this setting is both until the settings vs methods discussion is finalized.

My proposal is to keep the feature gate in this scenario because the users we have more who look at the docs and get confused because this setting is a no-op without feature gate would be more than the users who would use this setting in the reflection API.

@kbknapp
Copy link
Member

kbknapp commented Nov 16, 2021

I'm going to just make the tie breaking call that we remove the feature gate, but add in the docs that it's a hold over/no-op without the appropriate color cargo feature. You could even say something about how this feature will overhauled in future releases.

This is the best compromise so we can make progress with this merge and readdress in the future.

epage pushed a commit to epage/clapng that referenced this pull request Dec 4, 2021
)

Until we have a modular help generator that can be configured and/or
authored by the users themselves as part of #2914, we will provide the
flexibility of turning off colored help messages but still wanting
colored error messages.

This flexibility was available before #2845 and @dbrgn immediately
noticed it and requested it back to which I agree. This was also
suggested by Josh in
[here](clap-rs/clap#2806 (comment))
epage added a commit to epage/clapng that referenced this pull request Dec 4, 2021
Add DisableColoredHelp setting to improve flexibility (clap-rs/clap#2956)
@pksunkara
Copy link
Member Author

bors r+

I am merging since everyone agreed on the feature gate which was the last thing.

@bors
Copy link
Contributor

bors bot commented Dec 7, 2021

Build succeeded:

@bors bors bot merged commit 72e6b80 into master Dec 7, 2021
@bors bors bot deleted the disable-colored-help branch December 7, 2021 00:29
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.

4 participants