Config option to disable warnings

[bevel-zgates]

My use case ends up with a lot of overlapping entries as I tag a certain timeperiod multiple times and instead of passing --no-warn each time I would like to just throw this into a configurable and be done with it.

• extended args.NoWarn flag with a new config HideWarnings
• Documented in klog config calls
• Added quick test case

[jotaen]

Thanks for submitting this, this is actually something I also had in mind already.

My only note is that for a permanent config option, I think I’d like to make it so that the warnings can be controlled more fine-granularly, i.e. by warning type. I’m worried that it may be too broad a setting otherwise.

I’m thinking along the lines of:

# Only disable warnings about overlapping time ranges
no_warn = OVERLAPPING_RANGES

# Disable multiple warnings
no_warn = OVERLAPPING_RANGES, FUTURE_RECORDS

(There are 4 types of warnings right now.)

In case you are motivated to continue working on this, please feel free to do so. I’m otherwise also happy to take over this branch, either at this point or at a later stage.

[bevel-zgates]

Alright I've got it shimmed now.

Few questions. Right now I'm using the all caps snake case OVERLAPPED_RANGE for the config entries, then standardizing them with convertToCheckerName. This makes is nice for us in figuring out which strings get tied to which checkers, but has the unintended effect of leaking these checker names if the user runs klog config.

echo "no_warnings = OVERLAPPED_RANGE" >> ~/.config/klog/config.ini
klog config
> no_warnings = overlappingTimeRanges

I also have the application panic on invalid configs here if the typed in warning doesn't exist (or I can coerce it into an existing checker:

echo "no_warnings = OVERLAPPED_RANGE,something_misguided" >> ~/.config/klog/config.ini
klog config
> Error: Invalid config file

[bevel-zgates]

Want it as is or should I reverse translate the config for klog config calls?

[jotaen]

Awesome, this looks great! I’ll check it out this week.

Want it as is or should I reverse translate the config for klog config calls?

I’m not sure I follow, can you clarify?

[jotaen]

I’ve pushed a few changes:

• Added a bunch of tests
• Make the checker name a property of the checker struct. I think it’s a great idea to decouple the string identifiers from the code symbols. Instead of having a separate mapping function, I added a Name() method to the checker interface, so that the checker basically knows its own canonical name.
• I’ve introduced a type for disabled checkers and made it a set, so that we can make a cheap lookup inside the CheckForWarnings loop.

Could you look over the changes again, and – maybe more importantly –, briefly test them out on your end? I think we’d be good to go then.

Regarding the errors: currently, klog errors out for any config value that is invalid, so by doing the same here we’d be in line with the existing behaviour.

[bevel-zgates]

Looks good here. I debated the .ToLower() and if it should be more forgiving for user input, but I think with our error messaging and making it consistent is the better approach.

🚢 🚢

[bevel-zgates]

Awesome, this looks great! I’ll check it out this week.

Want it as is or should I reverse translate the config for klog config calls?

I’m not sure I follow, can you clarify?

While I had the .toLower() in the input validation a user would see a different value when they called klog config then what they would see in the config.ini. However, your patch 307c874 simplified this, so its a non issue now.

[jotaen]

Ah I see! Yeah, I agree it’s better to be strict with the casing, it’s also that way for the other config properties.