Re: String Equality Comparison, Broken Tests and .NET-1.x

[Jonas . Baehr]

Stefan Bodewig  wrote on 23.08.2016 06:14:32:

> Von: Stefan Bodewig 
> An: "Log4Net Developers List" 
> Datum: 23.08.2016 06:14
> Betreff: Re: String Equality Comparison, Broken Tests and .NET-1.x
> 
> On 2016-08-22,  wrote:
> 
> > A recent commit [1] changed, among other things, some string equality
> > comparisons from `SomeComparer.Compare(a, "B", IgnoreCase) == 0` to
> > `a.ToUpperInvariant() == "B"`, see also [2].
> >
> > Unfortunately, this doesn't work if `a` is allowed to be null. 
Currently a
> > lot of log4net.Tests are broken because of such a null reference 
exception
> > in `NewLinePatternConverter.ActivateOptions` (apparently "%newline" is
> > quite common in pattern layouts ;-).
> 
> Oh, I'm sorry. I must admit I glanced over the PR and applied it without
> running the tests. My fault.
> 
> > For new code I tend to opt for `String.Equals(Option, "DOS",
> > StringComparison.OrdinalIgnoreCase)` for a fast, case-insensitive
> > comparison with fixed ASCII-only patterns, but static
> > `String.Equals(String, String, StringComparison)` is not awailable 
on
> > .NET-1.x [3].
> 
> This is what the original code before PR #16 looked like, but it doesn't
> seem to be available for .NET Core, see the discussion around
> https://github.com/apache/log4net/pull/16/
> files#diff-51624ab11a9b3d95cc770de1a4e1bdbc

Note quite, it used `string.compare(string, string, bool, CultireInfo) == 
0` which is available on .NET-1.x, while `String.Equals(string, string 
StringComparison)` and `ToUpperInvariant` are not.

> > Should we create some helper in SystemInfo that provides null-aware,
> > ordinal, casing-agnostic string equality comparison, with some #if's
> > .NET-1.x?
> 
> +1

Here you go. The attached patch introduces a `
SystemInfo.EqualsIgnoringCase(string, string)`, some unit tests, and fixes 
`NewLinePatternConverter.ActivateOptions` so that the test suite passes 
again.

Please note that I was only able to test with .NET-4.5.2. I have no 
.NET-1x around, nor .NET Core (maybe we can even drop this #elif). I used 
the code for these platforms from previous revisions of 
NewLinePatternConverter.cs. In addition, I'm not sure if I got all the 
defines for the #if right. Is there some doc for that?

regards,
Jonas




SystemInfo_EqualsIgnoringCase.patch
Description: Binary data

Re: PluginMap survives LoggerRepositorySkeleton.ResetConfiguration()

[Jonas . Baehr]

Stefan Bodewig  wrote on 23.08.2016 06:05:23:

> Von: Stefan Bodewig 
> An: "Log4Net Developers List" 
> Datum: 23.08.2016 06:06
> Betreff: Re: PluginMap survives 
LoggerRepositorySkeleton.ResetConfiguration()
> 
> On 2016-08-22,  wrote:
> 
> > The question is: Is this the right way to do, or are there strong
> > reasons for the plugins to stay?
> 
> TBH I have no idea.
> 
> It might be the case that some people out there rely on this
> behavior. One indicator for this might be that you are the first person
> after twelve years who wants to change it :-)

Well, `someRepository.Plugins.Add(somePlugin)` silently replaces a 
potentially existing one (including shutdown), so when I just reset the 
configuration and add all my plugins again, I won't notice the missing 
plugin reset at first.
I also assume that log4net plugins are not that common.


> If this causes problems for your use case I'd recommend adding another
> reset method that also removes the plugins - and clarify the
> docs-strings for either method.

I'm against cluttering the API with several, nearly identical, methods. In 
my eyes, this will cause more confusion then benefit.

I'm using the methods in question in this way:
--8<---8<---
someRepo.Shutdown();
someRepo.ResetConfiguration();
foreach (var plugin in someRepo.Plugins.AllPlugins)
someRepo.Plugins.Remove(plugin);
reconfigure...
--8<---8<---

The problem is not so much the extra step of removing the plugins; its 
just annoying. The point is that `Shutdown()` is required to safely close 
all appenders (I'm using `Hirarchy` as repository). But the base 
implementation of `Shutdown` also shuts down all the plugins. Which is 
right, but now the plugins are not usable but still present after 
`ResetConfiguration`. I think this is a pitfall that should be cleaned up.

I can imagine two use cases:
- Shutdown before quitting the application
- Shutdown before reconfigure (therefore: reset configuration)
I consider Resetting without a prior shutdown an error. I see no reason to 
keep shutdown plugins in the repo after reset.
Also note that Hirarchy shuts down as part of reset, while the skeleton 
does not. But that is a problem of virtual methods calling each other and 
another story.


Regards,
Jonas