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

Unreachable code in StringArgumentValue / review support for dynamic config changes #99

Closed
MV10 opened this issue Apr 17, 2018 · 2 comments · Fixed by #160
Closed

Unreachable code in StringArgumentValue / review support for dynamic config changes #99

MV10 opened this issue Apr 17, 2018 · 2 comments · Fixed by #160

Comments

@MV10
Copy link
Contributor

MV10 commented Apr 17, 2018

(Figured I'd re-use this issue thread I opened accidentally... lol)

Anyway, the ConvertTo method in the StringArgumentValue class has unreachable code.

At the start of the method (lines 37-40) it converts the argument value when the requested type is LoggingLevelSwitch:

if (toType == typeof(LoggingLevelSwitch))
{
    return declaredLevelSwitches.LookUpSwitchByName(argumentValue);
}

Then way down at the end of the method (lines 116-139) there's a larger, completely different block of code for processing LoggingLevelSwitch.

I haven't done anything too complicated with the level switches so I'll leave it to somebody else to figure out if the second block is just old leftover code, or if they need to be combined somehow, etc.

@MV10 MV10 changed the title Bugs relating to use of ConfigurationSection.Value Opened in error Apr 17, 2018
@MV10 MV10 changed the title Opened in error Unreachable code in StringArgumentValue Apr 17, 2018
@tsimbalar
Copy link
Member

Ouch, this may be a regression I introduced as part of #88 :-/

I think there was at some point some support for detecting changes to the config file and updating the value of LoggingLevelSwitches , but I'm not sure exactly what the expected behavior was ...

Maybe @skomis-mm knows a bit better ?

@skomis-mm
Copy link
Contributor

Yes, it was for dynamic filtering control before LoggingLevelSwitches was introduced. Like MinimumLevel does:

"configureLogger": {
            "WriteTo": [
              {
                "Name": "LiterateConsole",
                "Args": {
                  "outputTemplate": "[{Timestamp:HH:mm:ss} {SourceContext} [{Level}] {Message}{NewLine}{Exception}"
                }
              }
            ]
          },
"restrictedToMinimumLevel": "Debug"

So changes to restrictedToMinimumLevel would result in change in underlying level switch. I think after #88 it is kinda deprecated now though it also introduced some breaking change in behavior. I believe this code should be moved and adapted to level switches declaration section so we can support of IChangedToken notifcations:

 "LevelSwitches": { "$mySwitch": "Warning" },

@tsimbalar tsimbalar changed the title Unreachable code in StringArgumentValue Unreachable code in StringArgumentValue / review support for dynamic config changes Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants