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: add TargetFilter::level_for_not #47

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Conversation

andylokandy
Copy link
Contributor

@andylokandy andylokandy commented Aug 14, 2024

No description provided.

@andylokandy andylokandy requested a review from tisonkun August 14, 2024 08:39

/// The filter will be applied only if the target **does not have** a prefix that matches the
/// target of the log record,
pub fn level_for_not(target: impl Into<Cow<'static, str>>, level: log::LevelFilter) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Read challenging. IIRC it could be generally neg_level_for or negtive_level_for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not negative on level but negative on for. So I'd like to put not word after for.

Copy link
Contributor Author

@andylokandy andylokandy Aug 14, 2024

Choose a reason for hiding this comment

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

Maybe level_excluding, level_for_excluding?

Copy link
Contributor Author

@andylokandy andylokandy Aug 14, 2024

Choose a reason for hiding this comment

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

In practice, it is like:

let dispatch = Dispatch::new()
    // discard other targets
    .filter(TargetFilter::level_for_not(
        "databend::log::structlog",
        LevelFilter::Off,
    ))
    .append(structlog_log_file);
logger = logger.dispatch(dispatch);

Copy link
Contributor

@tisonkun tisonkun Aug 14, 2024

Choose a reason for hiding this comment

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

I'd prefer to:

pub fn negative(mut self, negative: bool) -> Self {
  self.negative = negative;
  self
}

Maybe name it not, I'm OK with it. But the pattern is to invert the matching result manually in a fluent method. What do you think?

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

The example looks reasonable. I'm OK with either way now. You make the final call and let's try it out.

@andylokandy andylokandy merged commit 8fd1f34 into fast:main Aug 14, 2024
4 checks passed
@andylokandy andylokandy deleted the dev3 branch August 14, 2024 10:10
Comment on lines +53 to +54
let matched = metadata.target().starts_with(self.target.as_ref());
if (matched && !self.not) || (!matched && self.not) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bikeshedding - this should be logically matched ^ self.not 🤣

Copy link
Contributor Author

@andylokandy andylokandy Aug 14, 2024

Choose a reason for hiding this comment

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

Yes. I did intended. It help my brain be sane. XD

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