Reject Empty path values

[AraHaan]

Fixes #257.

[AraHaan]

On Merge please squash this to a single commit that matches the new name for this PR.

[nblumhardt]

Thanks for the PR!

== null is the correct intended check in most of these places; where we are accepting file names, if we use string.IsNullOrEmpty() we should switch to throwing ArgumentException rather than ArgumentNullException.

[AraHaan]

Alright, will do.

[AraHaan]

Might have to update tests a little after this change.

[bartelink]

Hi @AraHaan I'm doing some spring cleaning here

If you're intending to resolve the issues and take this PR to it's conclusion, it'd be good to get things resolved sooner than later

If you don't have capacity to do that in the short to medium term, can I request that this gets closed and then revisited from first principles

In general my review would align what's gone so far - having null checks is great; conflating null vs Empty vs whitespace is pretty much always bad news

[bartelink]

The OP cites #257, which is closed.
I am also closing #287, pointing to this as the replacement tracking issue
It would be good for the overview here to cite a clear aim in terms of what's being resolved

[AraHaan]

I will look into this later on either today or tomorrow in order to get this resolved as soon as possible.

[AraHaan]

Sorry about the late response, will get ready for some progress later on today.

[AraHaan]

Sorry for being late again, something came up and I had to do that before making the changes here.

Although I feel that output template check code could be optimized further to something like this:

outputTemplate ??= string.Empty;

Because it is more optimized then an == null check followed by a throw of ArgumentNullException. Basically I am saying that it should be fundamentally treated the same as a passed in empty string.

Documentation on the ??= operator: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-coalescing-operator

[bartelink]

this (and equivalents) invalidates the comment (and equivalents):

<exception cref="ArgumentNullException">When <paramref name="path"/> is <code>null</code>

[AraHaan]

Alright, will fix the comments as soon as possible.

[bartelink]

I personally think it would be useful for the existing check and comment to remain as-is for clarity
That way, there's no ambiguity as to what can cause the ArgumentException
(I also wonder if there should be a message indicating that the constraint is that Empty is not permitted and/or the xmldoc should mention it? Right now you're handed an exception and the only reasonable way to figure out the constraint is to read the source?)

[bartelink]

Sorry for being late again, something came up and I had to do that before making the changes here.

That's absolutely not a problem - the most important thing is that the code and user experience remains as boring as possible. Please take as much time as you want or need; nobody is tied to any date. (The reason I asked for the state is to simply figure out whether it was on the road to being abandoned, or the road to completion)

Although I feel that output template check code could be optimized further to something like this:

I'm not understanding your reasoning. The current code:

if (outputTemplate == null) throw new ArgumentNullException(nameof(outputTemplate));

Is clear and unambiguous - you MUST supply an output template. The point is not about efficiency, but clarity.

Efficiency has nothing to do with it - we are checking in case something went wrong such as not having null checks turned on.

Silently replacing a critical input with one that makes it silently write nothing would be very suprising and annoying to my mind. kinda like ON ERROR RESUME NEXT ;)

Am I missing something?

[bartelink]

Sorry to be a pain, but following up on #258 (comment)

• Issue Use Path.GetFullPath() to check for full path of the input path in FileLoggerConfigurationExtensions. #257, the original motivation for this is now closed.
• This leaves us without an overview comment that explains why this is happening/needed in the first instance

Can you explain in short (ideally by editing it into the overview in a clear way for everyone) and/or an extended reason down here why this check is required?

i.e. while a path of "" will mean files get named ".log, "001.log", which is wierd, does something more interesting actually go wrong?

• how would the situation arise, other than someone's config file having an empty value?
• even if that did arise, would it really be bad for them to end up with the strange name the asked on a garbage in- garbage out basis?

In general, the main concern for me is null values - empty is just a short string. i.e. if you were accepting a count, then 0 might be a perfectly legitimate value that can just be treated like any other integer?

[AraHaan]

It was originally about fixing that issue, but then I realized a simple misconfiguration of the logging config file resulted in the path not properly being used then closed that issue, but left this pr up as I also noticed that rejecting empty path values can be made as inputting an empty string can result in an exception when it tries to use file I/O to write the logs (it would actually try to write to a file with no name and no file extension and fail) the last time I had that issue which was about 1 year ago now, after that I started to do clever ways of defining the log file name by hard coding it in the program. And it was something that the null check missed.

[bartelink]

Sounds like things are stirring around here...
So am I to take it that the best thing here is to abandon this PR and see if anynoe runs into similar, or it's not such a big problem after all ?

[nblumhardt]

Closing this one as stale, but thanks for taking the time to check this out 👍