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

Check if the log entry is null #378

Closed
ChristianGreiner opened this issue Aug 21, 2016 · 2 comments
Closed

Check if the log entry is null #378

ChristianGreiner opened this issue Aug 21, 2016 · 2 comments
Labels
Bug It's broken and should be fixed Core Area: Duality runtime or launcher Help Wanted Contributions especially appreciated
Milestone

Comments

@ChristianGreiner
Copy link
Contributor

The Editor throws an exception when you write an "null"-string in the Write-Method of the logging system.

For example:

Log.Editor.Write(null);

I fixed this by checking if the msg is the IsNullOrEmpty:

// in Log.cs - Line 173
private void Write(LogMessageType type, string msg, object context)
{
      if (string.IsNullOrEmpty(msg)) return;
      ...
}

https://github.com/AdamsLair/duality/blob/master/Duality/Utility/Log/Log.cs#L173

@ilexp ilexp added Bug It's broken and should be fixed Core Area: Duality runtime or launcher labels Aug 21, 2016
@ilexp ilexp added this to the General Improvements milestone Aug 21, 2016
@ilexp
Copy link
Member

ilexp commented Aug 21, 2016

Distantly related to issue #327.

Your fix should work, but not doing anything on null or empty messages might lead to some debugging confusion. Instead, this might be a more transparent fix:

if (msg == null) msg = "[null message]";

Alternatively, an ArgumentNullException could be thrown in order to alert the developer of the fact that he isn't supposed to pass null as first argument:

if (msg == null) throw new ArgumentNullException("msg");

Not yet sure which variant would be better. Probably the first one though, since a logging operation is usually expected to never throw.


Edit: Care to submit a PR on this? It'd be faster and easier if I do this myself, but it would be an opportunity to get to know the PR procedure if you're (or someone else is) interested. You'd basically:

  1. Fork Duality on GitHub.
  2. Create a new branch (based on master) for your PR on your GitHub Fork.
  3. Check out the branch on your machine.
  4. Implement the fix in this branch.
  5. Commit & Push the branch back to your GitHub fork.
  6. Create a new PR from that branch to Duality-master.

@ilexp ilexp added the Help Wanted Contributions especially appreciated label Aug 21, 2016
@ilexp
Copy link
Member

ilexp commented Aug 27, 2016

Fixed.

@ilexp ilexp closed this as completed Aug 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It's broken and should be fixed Core Area: Duality runtime or launcher Help Wanted Contributions especially appreciated
Development

No branches or pull requests

2 participants