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

LogMessage Suggestion #31

Closed
LeeReid1 opened this issue Oct 8, 2021 · 3 comments
Closed

LogMessage Suggestion #31

LeeReid1 opened this issue Oct 8, 2021 · 3 comments

Comments

@LeeReid1
Copy link

LeeReid1 commented Oct 8, 2021

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
Copy link
Contributor

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
Copy link
Author

LeeReid1 commented Oct 8, 2021

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants