-
Notifications
You must be signed in to change notification settings - Fork 114
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
Duplicate body/asteroid loader fix #302
Conversation
Duplicate bodies/roids cause duplicate loggers to be created, throwing IOExceptions and killing loading. Now loader skips duplicated entries gracefully.
} | ||
catch (Exception e) | ||
{ | ||
bodyLogger.LogException(e); | ||
Logger.Default.Log("[Kopernicus]: Configuration.Loader: Failed to load Body: " + bodyNode.GetValue("name")); | ||
//bodyLogger.LogException(e); -- This would be nice but becomes a problem when duplicate loggers are created: can't log then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you already check for a duplicated body before the try / catch block starts, this could stay as it is right now. The logger shouldn't throw because of duplicated names anymore, so it doesn't need to be created in a try / catch.
The whole loading process seems to be aborted when a single log file fails to be created. This is why I covered it. |
Then please declare the bodyLogger variable outside of the try / catch but assign it inside. That way, the catch block can do a null-check on it and still log errors that are related to loading the body in the body log. |
The problem happened when the object was constructed not when it was set as active logger. Maybe we can create a simple constructor for it, then we can do as you suggest safely. |
Or we just do this
|
I'm not that good at C# yet, does a constructor that throw an exception set the instance to null? |
Yes, thats what happens. The class cannot be initialized so the variable retains the previous value (which is null by default) |
Logger class can now be instantiated without a filename and switch filenames. Loader now uses a single Logger instance.
Works exactly as before with less garbage collection, no null-checks and a smarter logger. |
Looks good, thank you! Merging |
Thanks for the feedback. |
Duplicate bodies/asteroids cause duplicate loggers to be created, throwing IOExceptions and killing loading. Now loader skips duplicated entries gracefully.