-
Notifications
You must be signed in to change notification settings - Fork 126
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
Logging refactor #504
base: main
Are you sure you want to change the base?
Logging refactor #504
Conversation
Yup, glad to see a change on that 👀
I would keep the logs in a single file. Given the log prefix one can still strip out certain parts by e.g. using |
The main reason i want to wrap them in a folder is separating the crash message into a separate file. Perhaps it could even be a json so that other apps could parse it
I can see some users really struggling with this. Even with improving error handling ( script errors, ... ) this would still be a problem. |
If the crash is caused by a mod, then having information about which mods are loaded in the same file would be quite useful in my eyes ^^ I assume whether it's stored in one file or multiple can be easily adjusted by just modifying the code related to the logger backend? In which case switching between the two should be rather simple once we decide which mode to choose (I'd still argue for single file :P) |
spdlog makes it really easy to switch between the configurations, so for the sake of testing i could implement it so that we know which one has more pros |
I'd say this is pretty much done, just waiting on #477 |
} | ||
|
||
SQObject functionobj {}; | ||
int result = sq_getfunction(m_pSQVM->sqvm, funcname, &functionobj, 0); | ||
if (result != 0) // This func returns 0 on success for some reason | ||
{ | ||
NS::log::squirrel_logger<context>()->error("Call was unable to find function with name '{}'. Is it global?", funcname); | ||
Error(SQ_GetLogContextNative(context), NO_ERROR, "Call was unable to find function with name '%s'. Is it global?\n", funcname); |
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.
This fixes #485
(Also I just noticed that the label for dependent PR was never removed when #477 was merged causing this PR to be stuck in PR hell, ugh. We really need some way to automate this as otherwise it will become impossible to keep track of stuff in the long run) |
Refactor to logging to make it more readable ( eg currently you can use both
spdlog::info
andNS::log::NS->info
, this is bad )Rough list of things: