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

feat(level): add off, debug, and trace options for default Level #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pitoniak32
Copy link

@pitoniak32 pitoniak32 commented Feb 19, 2024

add example for chaning the default, and mention in the docs

See #10 for some prior discussion

src/lib.rs Outdated Show resolved Hide resolved
examples/default_level.rs Outdated Show resolved Hide resolved
examples/default_level.rs Outdated Show resolved Hide resolved

/// Default to [`None`]
#[derive(Copy, Clone, Debug, Default)]
pub struct OffLevel;
Copy link
Member

Choose a reason for hiding this comment

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

In #10, we talked about how to handle --quiet but neither the code nor the description addresses this.

Copy link
Author

Choose a reason for hiding this comment

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

I added the suggestion of hide

value_parser that is assigned to a function that conditionally returns UnknownArgumentValueParser when L::default().is_none()

For this one I looked around the docs, and am not very familiar with clap value parsers, or how to use them conditionally in conjunction with actions. I can keep investigating this, I just wanted to address the other items first, and maybe get some guidance here.

Copy link
Member

Choose a reason for hiding this comment

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

This would be something like:

fn default_value_parser::<L: LogLevel>() -> clap::builder::ValueParser {
    if L::default().is_some() {
        clap::builder::UnknownArgumentValueParser::suggest("'--quiet' is the default").into()
    } else {
        clap::builder::value_parser!(u8).into()
    }
}
...
#[arg(value_parser = default_value_parser::<L>())]

Still not entirely sure its the right call to make but starting with an error is the safer choice as we can stop erroring without a breaking change but to start erroring is a breaking change.

Hmm, maybe instead we should make LogLevel in charge of this. If we provide inherent impls, then its generally recognized as not a major breaking change.

This would instead look like

pub trait LogLevel {
    // ...

    fn hide_quiet() -> bool {
        Self::default().is_none();
    }
    fn hide_verbose() -> bool {
        false
    }
    fn quiet_value_parser() -> bool {
        if L::default().is_some() {
            clap::builder::UnknownArgumentValueParser::suggest("'--quiet' is the default").into()
        } else {
            clap::builder::value_parser!(u8).into()
        }
    }
    fn verbose_value_parser() -> bool {
        clap::builder::value_parser!(u8).into()
    }
} 

@epage
Copy link
Member

epage commented Feb 19, 2024

Non-blocker: in the future

  • Please create new issue, rather than hijacking an issue
  • Please keep your commits atomic. This PR has three separate changes in one commit (tweaked documentation, adding an example with a call out to it, the trace levels)

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@pitoniak32
Copy link
Author

  • Please create new issue, rather than hijacking an issue

Apologies, I am new to making contributions to projects. I will note this for the future 👍🏼

  • Please keep your commits atomic. This PR has three separate changes in one commit (tweaked documentation, adding an example with a call out to it, the trace levels)

Sorry about that, I am getting bad commit habits from work. I have separated my commits in this PR.

I have addressed all but one of the concerns, I am planning on continuing to look into how to conditionally disable a flag with the suggested approach.

@epage
Copy link
Member

epage commented Feb 20, 2024

Apologies, I am new to making contributions to projects. I will note this for the future 👍🏼

It is definitely a different world to get used to on-board to faceless projects like this. Best way is to try and see what works. Glad you are here trying!

@joshka joshka mentioned this pull request Sep 9, 2024
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