Skip to content

Add a callback on the reader options to expose the log filter switches - #366

Merged
skomis-mm merged 2 commits into
serilog:devfrom
0xced:LogFilterSwitches
May 8, 2023
Merged

Add a callback on the reader options to expose the log filter switches#366
skomis-mm merged 2 commits into
serilog:devfrom
0xced:LogFilterSwitches

Conversation

@0xced

@0xced 0xced commented Mar 15, 2023

Copy link
Copy Markdown
Member

Just like it was done for log level switches in #352.

@nblumhardt nblumhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@skomis-mm skomis-mm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall 👍 Just a minor concern around $ prefixing on callback invocation.


_resolutionContext.AddFilterSwitch(switchName, filterSwitch);
var referenceName = _resolutionContext.AddFilterSwitch(switchName, filterSwitch);
_resolutionContext.ReaderOptions.OnFilterSwitchCreated?.Invoke(referenceName, filterSwitch);

@skomis-mm skomis-mm Mar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the $ sign make sense only for a sink parameter in appsettings.json to distinguish parameter value from variable reference:

"Filter": [
  {
    "Name": "ControlledBy",
    "Args": {
      "switch": "$filterSwitch"
    }
  } 
]

And since #231 $ is optional in declaration section, leaving name as is should look more consistent with overrides.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@0xced any thoughts on this one? (I'm working on merging everything through for a v7 shortly, would be great to get this PR in however this goes)

@0xced 0xced May 6, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I haven't forgotten about this. I will reply soon but I want to first experiment with a real world scenario to figure out what's best.

@0xced 0xced May 7, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree with you that there's no need to force the leading $. So I addressed this in 7f5c27f.

@0xced
0xced force-pushed the LogFilterSwitches branch from 4b52550 to 3fea9f7 Compare May 5, 2023 09:02
@0xced
0xced force-pushed the LogFilterSwitches branch from 3fea9f7 to d22b9fe Compare May 5, 2023 14:22
@0xced 0xced mentioned this pull request May 7, 2023
@0xced
0xced force-pushed the LogFilterSwitches branch from f8813e9 to 7f5c27f Compare May 8, 2023 07:09
@skomis-mm
skomis-mm merged commit 26945f1 into serilog:dev May 8, 2023
@0xced
0xced deleted the LogFilterSwitches branch May 8, 2023 15:30
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