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

Comment style #53

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

whilefoo
Copy link

@whilefoo whilefoo commented Jan 6, 2025

Resolves #50

@whilefoo
Copy link
Author

whilefoo commented Jan 6, 2025

@0x4007 @gentlementlegen

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Redundant entries should be replaced.

error: "!", // ! text in orange
info: "#", // # text in gray
debug: "@@@@", // @@ text in purple (and bold)@@
private _diffColorCommentMessage(type: LogLevelWithOk, message: string) {
Copy link
Member

Choose a reason for hiding this comment

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

It is also no longer called diff

@whilefoo
Copy link
Author

whilefoo commented Jan 7, 2025

Redundant entries should be replaced.

We have more logger methods than possible formattings. I'd remove verbose because I think it's semantically the same as debug, but fatal is different than error because it displays the metadata instead of hiding it in the comment

@gentlementlegen
Copy link
Member

Tested here: Meniole/ubiquity-os-kernel#20

Works fine. Maybe it is strange to have Important for debug and might be better without any header, and use Important for verbose?

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

Tested here: Meniole/ubiquity-os-kernel#20

Works fine. Maybe it is strange to have Important for debug and might be better without any header, and use Important for verbose?

We can try your suggestions and iterate after.


I still think that we should use client error and server error because that is not ambiguous and all plug-in developers are more likely to agree on the definitions and conform to the standard

@gentlementlegen
Copy link
Member

What is server and what is client? Because the kernel, the Actions and the Workers can all post errors. I think this should be the responsibility of the developer to add these in the metadata and not in the message itself

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

#50 (comment)

@gentlementlegen
Copy link
Member

I think this should be in the metadata, it doesn't give any useful information to the user, and for the debug we now have the url inside of the metadata and can easily figure out what broke. A logger should be generic and able to log anything without depending on the platform or environment, which is why we could embed this info in the metadata (which becomes the SDK responsibility)

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

A logger should be generic and able to log anything without depending on the platform or environment

Generally, I agree, but when I first implemented this, I was trying to solve a problem of logging in three separate places:

  • the console, for local development
  • GitHub comment, for triage
  • The database, for later review

Due to these requirements this was designed specifically for this platform and environment (UbiquityOS plugins, interacting with GitHub)

@whilefoo
Copy link
Author

whilefoo commented Jan 8, 2025

Works fine. Maybe it is strange to have Important for debug and might be better without any header, and use Important for verbose?

So debug would be in plain-text (raw)?


I still don't see the benefit in separating error into clientError and serverError, it's not useful either to the user or developer. Client errors only happen when using commands incorrectly and I think it's descriptive enough to use logger.error("Provided address is in incorrect format")

@gentlementlegen
Copy link
Member

@whilefoo yes or contained within a code block? But that was just a suggestion I don't mind it the way it is, just that we maybe want debug to stand out because it is non-normal logs.

@whilefoo
Copy link
Author

whilefoo commented Jan 8, 2025

@whilefoo yes or contained within a code block? But that was just a suggestion I don't mind it the way it is, just that we maybe want debug to stand out because it is non-normal logs.

Wouldn't making it without header make it stand out less :D
It seems verbose is only used text-conversation-rewards where it's logging a stringified JSON so maybe that's more suitable for a code block?

@gentlementlegen
Copy link
Member

@whilefoo Fair enough haha. Yes across our plugins I don't think verbose is used much, maybe in command-start-stop as well? It is hard to tell when this should be chosen instead of ok or info. In my thoughts it was just because I wouldn't expect debug messages to actually get posted to an issue, thus not needing the header.

@whilefoo
Copy link
Author

whilefoo commented Jan 8, 2025

In my thoughts it was just because I wouldn't expect debug messages to actually get posted to an issue, thus not needing the header.

That's true, debug and verbose usually aren't thrown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger Comment Formatting
3 participants