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

Style new const #60

Closed
wants to merge 2 commits into from
Closed

Style new const #60

wants to merge 2 commits into from

Conversation

jaudiger
Copy link

@jaudiger jaudiger commented May 22, 2024

If this PR is merged, I'll then be able to constify almost all the methods of "Color": see #61

@@ -262,7 +274,7 @@ impl Style {
/// assert_eq!(false, Style::default().bold().is_plain());
/// ```
pub fn is_plain(self) -> bool {
self == Style::default()
self == Style::new()
Copy link
Author

Choose a reason for hiding this comment

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

We cannot constify this method since the equal operand is not usable in const context

@jaudiger jaudiger mentioned this pull request May 22, 2024
@jaudiger
Copy link
Author

I opened another MR to demonstrate the propagation of const to other structs

@kubouch
Copy link

kubouch commented Jun 25, 2024

Is there some particular reason for constifying the methods (also in your other PRs)? I don't have anything against it, but it's always good to have a concrete use case when contributing something.

Thank you for your patience. At some point, we'll go through the PRs and make a new release of this crate.

@jaudiger
Copy link
Author

No real use case for the constify of these methods, I had to admit. I just saw an opportunity to rewrite some parts of the code (here through the usage of the "new" method in the default trait) to later enable more methods to be constified. I haven't looked to propagate those new const possibilities elsewhere in the code. But if you are welcome to see more MRs on this topic, I'm open to work on it.

@kubouch
Copy link

kubouch commented Jun 29, 2024

So since there does not seem to be a strong use case, we won't land these const PRs for now. In general, we don't want to change this crate except bugfixes and general maintenance. This crate has some 60M downloads, so any breakage would be quite significant, we don't want to take any chances.

We could reconsider it if people start complaining that they absolutely need to define const ANSI sequences.

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.

2 participants