nreco / logging

Generic file logger for .NET Core (FileLoggerProvider) with minimal dependencies
MIT License
284 stars 56 forks source link

LogMessage Suggestion #31

Closed LeeReid1 closed 2 years ago

LeeReid1 commented 2 years ago

Hi, Thanks for this little library.

I just have two minor suggestions regarding LogMessage.

The first is to consider making it a class. Perhaps there's a reason you can't use a class(?), but as a struct we can't extend it, and structs holding reference types can cause some difficulties to those who are unaware (remember that Exception is mutable).

The second is to make the constructor public (it's currently internal), or simply delete it entirely, so we can create LogMessages in our code. Without this, refactoring sometimes gets more awkward than it needs to be.

internal static string FormatLogMessage(string someargument)
{
    LogMessage lm = new LogMessage()
    {
        Message = someargument // ERROR
    };
    lm = new LogMessage(someargument);//ALSO ERROR
    return FormatLogMessage(lm);
}

internal static string FormatLogMessage(LogMessage arg)
{
...

Neither are at all critical but just something to consider for next time you work on the library.

Thanks again

VitaliyMF commented 2 years ago

I'm not sure that I understand the purpose of these suggestions re LogMessage -- it has internal constructor and cannot be extended simply because it is created inside FileLogger (and simply holds the context of the log entry) and that's all, externally it is available only as an argument for FormatLogEntry handler.

LeeReid1 commented 2 years ago

Well... like I said, it's not critical, but as a simple example, I can't easily unit test my formatter because I can't create LogMessage instances.


[TestMethod]
public void FormatterTest()
{
  // check stability handling nulls
  LogMessage lm = new(null, default(LogLevel), default(EventId), null, null); // COMPILER ERROR
  Assert.AreEqual(string.Empty, ErrorHandling.FormatMessage(lm));
}

Given that LogMessage does nothing except hold values, I don't understand the reason to prevent people from using the constructor for things like this.

By extended I meant inheriting from LogMessage for related uses, rather than reinventing the wheel (you cannot inherit from a struct). I don't have a need for this just yet, but preventing this by using a struct just seems like an unnecessary restriction in this instance.

Again, just suggestions.

VitaliyMF commented 2 years ago

LogMessage is not obligated to be struct/have internal constructor, I just wonder what could be the purpose of creating LogMessage outside the logging provider. If you need to have an ability to create LogMessage in your code feel free to submit a PR.