Explicitly Callable Functionality for Logging of App Activity Within CoreSDK #122
Conversation
…ore SDK and third-party apps via easylogging++. Contains base config and initialisation options but currently no way of setting levels (hierarchical model is not default for Easyloggingpp).
…ut it in its proper place.
|
@macite and @hugglesfox, we have a problem, potentially caused by backend logging support: now that I have exposed logging to the core SDK, it implicitly assumes that logging is supposed to be on all the time, hence the necessity of not explicitly calling the INITIALISE_EASYLOGGING macro to avoid compilation errors, as warned by the Easyloggingpp readme. Now every time I run a program for SIT102, a default my_easylogging log file is always created. How can we avoid this? |
|
I just had a quick look, and this approach isn't going to work. The idea inside SplashKit is to create a simplified interface for others to use. So you will need to create our own Logging interface - something that is simple and easy to use. Internally we can then use easylogging to do much of the work for us. So think about the features that we want to offer via logging, and some sensible defaults for things like the log format. Keep the focus on simplicity over large numbers of features. Also keep the interface to using just basic type in parameters. |
|
Okay so we keep the interfaces separate. Hopefully that will make things easier. So @macite, this means you expect implementation and deployment to be pretty much purely procedural? |
|
I decided to implement logging myself using the |
|
@hugglesfox, yes, looks pretty close to what I was thinking of implementing. Do you want to contribute to the PR from your own account for credit purposes, or should I just add the code in with appropriate adjustments, if / where necessary? |
|
@njsch I don’t mind. What ever is easiest. As long as the feature is implemented that’s the main thing. |
njsch
changed the title
…oaded for review, code doesn't work yet.
|
@macite, I'm having trouble compiling Hayley's code. I don't think it is possible to hash-include splashkit.h, so I included terminal.h for write() and write_line() but the compiler can't find them, even after explicitly using the splashkit_lib namespace. Output is below: |
|
I've fixed up a few things for you:
Looking good at the moment. Keep working on it. |
|
I've pushed changes to your branch, so you will need to pull these into your local repo. |
|
@macite, Thanks. |
|
You should be able to keep working on this in your logging branch. Best to isolate this change and finish it off before incorporating other changes. |
… to add to log files but due to syntax errors with utility_functions, one cannot compile and test the code.
|
@macite and @hugglesfox, added my latest changes for draft analysis but haven't been able to test due to a syntax error with utility_functions. I have attempted to add support for an output log file with a configurable status between console mode, file only mode or both a file and console output mode. |
|
@macite, @hugglesfox: Any suggestions for (a) appropriate memory clean up when closing a log output file; and (b) how I can stop the compiler from assuming that the third parameter of set_log_status(), passed by reference, should have a default argument value? |
|
a) Putting b)
https://en.cppreference.com/w/cpp/language/default_arguments But I couldn’t see a function override in that same scope? So not sure where it’s getting the default value from but reordering the arguments so that all the default arguments are last should fix it. |
There was a problem hiding this comment.
Looking really good. I've added some inline comments that will hopefully help progress this further.
Please check the naming convention of commit messages. You want them to be like FIX: Added close_log used to close log files, or NEW: Added additional log options, or QUALITY: Fixed up indentation issues in logging code etc.
FIX is for commits that fix some form of issue. NEW is for new functionality. QUALITY is for things that tidy up implementations without adding or fixing things.
Hope that helps.
|
@macite, why exactly are we using sk_init_logging in the backend, as the external logging module is separate from the easylogging implementation? |
|
@njsch given we need to initialise this logging module I think it fits - no need to have two separate initialisation processes. This would then be setup on the first use of anything that does require initialisation - which would include the logging functions. You would need to make sure to call the base init function in the logging methods that need this initialisation to have occurred. |
…egun to add some of @macite's changes.
|
Partially implemented @macite's requested changes but still unsure of a few things:
|
|
@macite, There is an error with my attempt at overloading. Why won't it accept the two as opposed to three possible argument scenarios? Errors below: |
|
@njsch looks like you have 2 overloads - one with one parameter and one with three parameters. There isn't one with two parameters. Also, remember you will need to implement all of the different versions of this. In the C++ code you have the most explicit version (the one with three parameters) but not the other. Each of the others will be a single line calling the version with three parameters, and providing default options for the values it did not receive. e.g. void init_custom_logger (string app_name, log_mode mode)
{
init_custom_logger(app_name, true, mode);
}I see you are passing the log_mode by reference. There is no need to do this. The code isn't updating the value passed to it, and it is already small in size as an enum (it will be the size of an integer). Removing the Hope that helps. |
…n <log_mode> enum to <FILE_ONLY> to avoid typedef conflict with standard lib reference to <FILE>. Fixed the logging test.
|
FYI @macite, immediately previous commit contributes the following:
|
|
@macite, @hugglesfox, correction from last comment: log file is being written to in ```splashkit-core/bin. But I'm not sure if the variable for setting the log level is necessarily doing its job. |
|
This is not working on Mac. I can't test / fix this reliably unless I get a Mac OS VM. |
|
Closing this for now as development seems halted on this project. FYI @macite. |
closes #115.
This enhancement takes advantage of the already-included external Easylogging++ code and allows it to be explicitly called by third-party apps using Splashkit, not just inside the SDK's internal code itself.
Intending to work on this with @hugglesfox. Currently it is in draft mode and untested, just putting this PR here for reference and to automatically close the issue I created when done.
At the moment, it is mildly configurable but it is impossible to set logging levels. I don't really understand how to do this when a hierarchical model is not used by default in Easylogging++.
Also still learning the ropes when it comes to headerdoc (which I believe also cannot be installed on Windows) so unsure as to where to put docstrings/documentation comments.