Allow to change the RuleName of a LoggingRule

[304NotModified]

Small enhancement for NLog 4.7.3

So we could

• rename one

• create easier an instance

  e.g. do this:

  var target = new FileTarget("t1");
  var loggingRule = new LoggingRule("MyNamespace.*", LogLevel.Info, LogLevel.Error, target);
  loggingRule.RuleName = "name1";
  

  instead of

  var loggingRule = new LoggingRule("name1");
  loggingRule.EnableLoggingForLevels(LogLevel.Info, LogLevel.Error);
  loggingRule.Targets.Add(target);
  loggingRule.LoggerNamePattern = "MyNamespace.*";
  

[304NotModified]

But maybe it's an issue as after reload (XML config) the rename is lost?

[snakefoot]

I see the RuleName as a RuleId. And it should remain static throughout the object-lifetime. The LoggingRule-RuleName has been implemented very loosely without uniqueness, because it was an afterthought. In the correct world then LoggingConfiguration should validate one is not adding multiple LoggingRules with the same name. And also allow dictionary-like lookup of LoggingRule on RuleName. But if suddenly allowing changes to LoggingRule.RuleName then this validation/lookup is very hard.

Just like Target-Name-setter should explode in a rain-of-exceptions when trying to modify the Target-Name after having added to LoggingConfiguration. But throwing exceptions when calling setters is usually not nice, but dictionaries for lookup will not reflect the name-change.

If there is missing a constructor or fluent-api for setting up LoggingRules, then that should be implemented for NLog 5. Instead of opening of for public-setters without handling name-collisions etc. Ex:

var loggingRule = new LoggingRule("name1");
loggingRule.AddTarget(target, LogLevel.Info, LogLevel.Error, "MyNamespace.*");

Where AddTarget is something like this:

        public void AddTarget(Target target, LogLevel minLevel = null, LogLevel maxLevel = null, LoggerNamePattern loggerNamePattern = null)
        {
              Targets.Add(target);
              LoggerNamePattern = loggerNamePattern ?? LoggerNamePattern ?? "*";
              if (_logLevelFilter == null)
              {
                   minLevel = minLevel ?? LogLevel.MinLevel;
                   maxLevel = maxLevel ?? LogLevel.MaxLevel;
                   EnableLoggingForLevels(minLevel, maxLevel);
              }
              else
              {
                  if (minLevel != null)
                  {
                      DisableLoggingForLevels(LogLevel.MinLevel, minLevel);
                      EnableLoggingForLevel(minLevel);
                  }
                  if (maxLevel != null)
                  {
                      DisableLoggingForLevels(maxLevel, LogLevel.MaxLevel);
                      EnableLoggingForLevel(maxLevel);
                  } 
              }
        }

Could also make it fluent by returning this instead of void.

[304NotModified]

And also allow dictionary-like lookup of LoggingRule on RuleName. But if suddenly allowing changes to LoggingRule.RuleName then this validation/lookup is very hard.

I'm aware of that, but I checked the code and there isn't a dictionary lookup? It's just a foreach loop.

[snakefoot]

I'm aware of that, but I checked the code and there isn't a dictionary lookup? It's just a foreach loop.

Because it was added at a late stage, so it had to handle unnamed rules. But could be nice with not adding duplicate rules (Maybe for NLog 5), and enforcing that is not easy when having a public-setter for RuleName (becomes fragile).

[304NotModified]

Funny, i'm fine with non uniqueness. I don't see things break if we allow that. (as we don't use the name for anything functional)

[snakefoot]

FindRuleByName kind of expects uniqueness. RuleName was just added in best-effort-style, because no-one else had the guts. And wanted to see in what direction the ball went. But making it more fragile with more unexpected behavior doesn't seem like a good direction.

var rule1 = new LoggingRule("hello1");
config.LoggingRules.Add(rule1);
var rule2 = new LoggingRule("hello1");
config.LoggingRules.Add(rule2);  // Should it overwrite rule1? Should alert and say something is bad?
var rule3 = new LoggingRule("hello3");
config.LoggingRules.Add(rule3);
var ruleX = config.FindRuleByName("hello1");  // What is going on?
config.LoggingRules.Last().RuleName = "hello1";
ruleX = config.FindRuleByName("hello1");  // What is going on?

I prefer the above madness already stops when calling config.LoggingRules.Add(rule2) and says the rule already exists (With string-compare-case-insensitive). Instead of giving a poor programmer grief for several days trying to figure out what is going on.

[304NotModified]

I don't see it like that. The name is optional and should not be used by NLog itself. So nothing goes wrong - it's just meta data. I think we should keep that, agree?

Maybe FindRuleByName should be renamed to FindFirstRuleByName in the future.

[snakefoot]

should not be used by NLog itself.

Then please remove the methods FindRuleByName and RemoveRuleByName. If you say RuleName is not used by NLog.

NLog is a framework. This means its methods should be predictable and easy to use. Not having traps and pitfalls. LoggingRules should disappear when they have been removed. Not when they have been renamed. RuleName could also have been RuleId. But because of Target-Name then it was decided to make them similar in naming.

Already think NLog Target Name is super fragile, and illogical. Ex. why can a target not be found by its name when using <targets async="true">? Have more of this goo is not a good path.

[snakefoot]

Anyway just merge it. Not that important. Since I'm not gonna use it. I guess you have great place where this hack is needed, and want to pay with some pitfalls for the users to moan about. 1000 papercuts later :)