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: 🐛 Contradicting argument configurations #147

Merged
merged 12 commits into from
Apr 27, 2023
Merged

Fix: 🐛 Contradicting argument configurations #147

merged 12 commits into from
Apr 27, 2023

Conversation

KP64
Copy link
Contributor

@KP64 KP64 commented Apr 26, 2023

Right now, there are the following output coloring options:

 /// Turn on colorization always
#[arg(short = 'C', long)]
pub force_color: bool,

and

/// Print plainly without ANSI escapes
#[arg(long)]
pub no_color: bool,

This allows Users to contradict themselves when passing arguments like this:

cargo run -- --no-color --force-color // It will force the color btw

Which is why I've come with the following solution:

#[derive(Clone, Copy, Debug, clap::ValueEnum, PartialEq, Eq, Default)]
pub enum Coloring {
    None,
    #[default]
    Colored,
    Forced,
}

and

#[arg(short = 'C', long, value_enum, default_value_t = Coloring::default())]
pub coloring: Coloring,

This also allows Users to specify the coloring output they want with “-C” alone, making it more intuitive.

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

This is much better. Thank you! Could you also update the usage section of the README to reflect the new arguments? I can update this part of the readme later once this is merged.

src/render/context/mod.rs Outdated Show resolved Hide resolved
This removes the Clippy lint "struct_excessive_bool" from the "allow" list, reactivating it
The struct still contains more than 3 bools giving a big warning.
@KP64
Copy link
Contributor Author

KP64 commented Apr 26, 2023

This is much better. Thank you! Could you also update the usage section of the README to reflect the new arguments? I can update this part of the readme later once this is merged.

Alright, I think I've done it. Have I missed a section or anything?

@ilyapopov
Copy link

I would suggest "auto" instead of "colored". Maybe also add a long alias --color.

@KP64
Copy link
Contributor Author

KP64 commented Apr 26, 2023

I would suggest "auto" instead of "colored". Maybe also add a long alias --color.

Indeed. This would probably be better. I'll change it right away!

src/render/context/mod.rs Outdated Show resolved Hide resolved
@solidiquis solidiquis merged commit 6913115 into solidiquis:master Apr 27, 2023
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.

3 participants