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

Add godot error messages #10

Conversation

Samuel-Lewis
Copy link
Contributor

@Samuel-Lewis Samuel-Lewis commented Oct 9, 2020

Adds option to include error codes in log functions. See README for usage. As one of the feature requests in #9.

Output for the log line

Logger.error('Failed to rotate the albatross', 'main', ERR_QUERY_FAILED)

Will look like:
Screen Shot 2020-10-09 at 2 56 34 pm

@SirMoM
Copy link
Contributor

SirMoM commented Oct 9, 2020

Having the error code and the module name as optional parameters has the problem that the latter is dependent on the first to always be filled. This means that the module name has to be set if I want the error message.
This might not be desired but since Godot does not have the option to fill only specific parameters I don't really see a way around it?
Maybe changing the order of the parameters would be beneficial since I assume the error codes might be used more than changing the module.

func error(message, , error_code = -1, module = default_module_name):

On another topic:
I would like to see a space between the closing bracket of the module and the error message.

var output_format = "[{LVL}] [{MOD}] {ERR_MSG} {MSG}"

@Samuel-Lewis
Copy link
Contributor Author

Oop, yep. Missed the space. I don't think changing it to "[{LVL}] [{MOD}] {ERR_MSG} {MSG}" would necessarily work though, because when an error is not specified, there would be an odd double space.

[INFO] [main]  File loaded!

I can optionally add the space if there is an error present. That might break some assumptions users have with how the output_format behaves, but it's fine for now.

As for the param order, I'm unsure which would be used more frequently, module or error code. When I was using the error codes, every log message had a module. But that's just my style. The main reason why I added it as the last param however, is that moving where the module is would result in a breaking API change for any existing users. Not the worst as I don't suspect there are a heap of users, but still not a change to make lightly. Thoughts?

@SirMoM
Copy link
Contributor

SirMoM commented Oct 10, 2020

Regarding the space: It can be added inside the format function should be fine. And it doesn't change the output_format

# Error message substitution
var error_message = error_messages.get(error_code)
if error_message != null:
	output = output.replace(FORMAT_IDS.error_message,  " " + error_message)
else:
	output = output.replace(FORMAT_IDS.error_message, '')

Regarding the parameters:
The only alternative I can think of is to have multiple wrapper functions and this doesn't appeal to me to be honest.
But yeah changing the API is a concern I haven't thought of.

@@ -6,6 +6,58 @@

extends Node # Needed to work as a singleton

const error_messages = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for the purpose of this plugin, but in the long run I think it might be worth having this kind of stringification of error codes directly in the engine itself, as many users would benefit from an easy translation of error numbers to human readable strings.

logger.gd Show resolved Hide resolved
logger.gd Show resolved Hide resolved
@akien-mga
Copy link
Member

Would there be use for this feature outside of error() calls? If not, maybe the error code could be added only to the error() prototype and formatted there as part of the message.

Though it would make it harder to customize the format (unless a new format template is added for error messages specifically).
Just throwing ideas, I'm not necessarily asking to change things.

Regarding the use of the API with the growing number of optional parameters, I share the concern. The 'module' system we designed initially feels quite awkward to me now, and I think a refactoring could be useful.

The module could be a class member and there could be different LoggerModule instances for each, so you could do e.g. Logger.server.error(...) to call error() on the "server" module registered as a member variable.
But that's a discussion to be had in a dedicated issue I guess.

@SirMoM SirMoM mentioned this pull request Oct 20, 2020
@SirMoM
Copy link
Contributor

SirMoM commented Oct 20, 2020

I have created an Issue #13 to discuss the modules further. So this PR can be merged ^^
@akien-mga

@Samuel-Lewis Samuel-Lewis force-pushed the samuel-lewis/error-strings branch from 2cd168c to bae6ed3 Compare November 4, 2020 20:48
@akien-mga
Copy link
Member

I rebased to squash commits and do various fixes to the documentation and the formatting of the code: 511164b

Since I couldn't force push to this PR (the checkbox to allow it was likely unticked), I pushed to master directly: 9d495b0

Thanks!

@akien-mga akien-mga closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants