-
Notifications
You must be signed in to change notification settings - Fork 416
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
Standardise logging calls #6099
base: master
Are you sure you want to change the base?
Conversation
I will leave this PR as a draft until I finish updating the log instances where required with these new parameters. |
On second thought, I think it's probably better to have this reviewed first! As otherwise I'd need to change up those logs again in case of review changes. |
Logger.Log("message", valueChanges: valueChanges, values: values); | ||
Logger.Log("message", "test", valueChanges: valueChanges, values: values); | ||
Logger.LogPrint("message", valueChanges: valueChanges, values: values); | ||
Logger.LogPrint("message", "test", valueChanges: valueChanges, values: values); | ||
Logger.NewEntry -= logTest; |
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.
dunno this is all a mess to me dunno what anything means in the args.
I have doubts about how useful this change is.
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.
Sorry, to clarify I followed the suggestion made here: ppy/osu#10682 (comment) to try and enforce the standard through the logger in the Framework.
Assuming you were referring to the functionality itself and not the test, looking back, I can see why the new arguments do not convey what it's supposed to be for (I guess the namings and/or having two separate dictionary arguments for similar but different purposes). Therefore, perhaps for the sake of the issue, I could simply just manually change up the relevant log lines in osu to follow the said standard without having anything additional in Framework?
Please advise. Thank you
Addressing ppy/osu#10682
The requirement is that we will try to standardise logs in the following format:
<timestamp> <log level> <calling class>, <event>, <key>:<value> <key>:<value> ...
The calling class is now recorded, though if required we can make this optional.
The
<key>:<value>
examples shown have two different variations:As a result, I've added two separate dictionaries respectively to support both; the first variation will be a
string
/object
dictionary for aname:value
input while the second variation will useobject
/object
to allow support forvalue changes
(e.g two integers, two strings, etc.).The
working beatmap: "Some beatmap name"
example was ignored (Even though it includes a little space after the:
) as it was probably meant to fall under the first variation.If both are specified into the log, then
from/to
takes precedence.This PR is currently to implement the functionality, but does not yet replace all instances where these two new optional parameters are to be used. I've added them to all methods and their overloads that uses the
add
method.