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.

Sign up for GitHub

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

Jump to bottom

Added support for debug logging #14

Open
wants to merge 1 commit into
base: typescript
Choose a base branch
from
Open

Added support for debug logging #14

wants to merge 1 commit into from

Conversation

Mattie112
Copy link

Just a suggestion to make it a bit easier to debug stuff (leave messages still don't work all the time for me).

@mikhailmikhalchuk
Copy link

Don't necessarily see the point of this - console already provides the info, debug, and error methods, and this pull request dose not implement any of them. Filtering what to log based on a config option is impractical.

@Mattie112
Copy link
Author

Why would that be impractical? That is how a lot of software works. Normal use case is either info or warning and then a debug for well debugging. You don't want to leave debug logging running all the time as (depending on the software ofc) this can cause for a lot of disk usage. The built-in console functions do not have anything to set a log level (as far as I know correct me if I'm wrong).

For example: I have issues with leave messages so I want to know what the bot is doing. Is it reading correctly from factorio (and then not sending?) Or is it not parsing? Or can't it post to Discord. So I want stuff that logs it all but for the normal flow that only wastes disk space.

Me not using console.debug or console.error is from my web dev background, that used to give a lot of problems on IE. I don't really mind using those functions but that is something different from wanting to set the log level.

@mikhailmikhalchuk
Copy link

mikhailmikhalchuk commented Jan 28, 2022 •
edited
Loading

Normal use case is either info or warning and then a debug for well debugging. You don't want to leave debug logging running all the time as (depending on the software ofc) this can cause for a lot of disk usage.

There's a config toggle on whether or not to send debug messages to the console, see config.logLines. You can use this to debug without the need for an enum.

Me not using console.debug or console.error is from my web dev background, that used to give a lot of problems on IE.

In this case, you could use console.debug for debugging and console.error for warnings/errors. Considering the introduction of the enum, this would be the more sensible option.

My concern is that this pull request adds a lot of unneeded bloat for a config feature that exists already or could simply be altered.

@AGuyNamedJens
Copy link
Owner

It could be an idea to make every log be able to be set, but except for logLines, which logs all incoming lines to the console, there isn't any other "debug" item to log unless you want to disable all connection logs, which in my opinion are quite important

@Mattie112
Copy link
Author

Normal use case is either info or warning and then a debug for well debugging. You don't want to leave debug logging running all the time as (depending on the software ofc) this can cause for a lot of disk usage.

There's a config toggle on whether or not to send debug messages to the console, see config.logLines. You can use this to debug without the need for an enum.

Me not using console.debug or console.error is from my web dev background, that used to give a lot of problems on IE.

In this case, you could use console.debug for debugging and console.error for warnings/errors. Considering the introduction of the enum, this would be the more sensible option.

My concern is that this pull request adds a lot of unneeded bloat for a config feature that exists already or could simply be altered.

But that config item really logs everything, the nice thing about log levels is that the user can decide how much logging he/she wants. But I agree that you want one of them, not both. I don't mind using the other console functions, doesn't change anything in the output for cli afaik.

It could be an idea to make every log be able to be set, but except for logLines, which logs all incoming lines to the console, there isn't any other "debug" item to log unless you want to disable all connection logs, which in my opinion are quite important

Well other stuff could be discord status checks, the parsing of messages for example. The more features you add the more stuff that (might) break.

@AGuyNamedJens
Copy link
Owner

Hmm, interesting..

mikhailmikhalchuk
mikhailmikhalchuk suggested changes Jan 29, 2022
View reviewed changes
Copy link

@mikhailmikhalchuk mikhailmikhalchuk left a comment

Choose a reason for hiding this comment

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

Regardless of whether this becomes merged, there are changes that need to be made.

.gitignore
mod_updater.py

.idea
Copy link

@mikhailmikhalchuk mikhailmikhalchuk Jan 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why was .idea added?

index.ts

import { Rcon } from "rcon-client";
import {Rcon} from "rcon-client";
Copy link

Choose a reason for hiding this comment

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

Convention is spaces between words and brackets.

index.ts
@@ -16,6 +16,13 @@ const bot = new Discord.Client({ intents: [Intents.FLAGS.GUILD_MESSAGES, Intents

const Commands: Discord.Collection<string, Command> = new Discord.Collection();

enum LOGLEVELS {
Copy link

Choose a reason for hiding this comment

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

Convention is order numerically. Additionally, should increment by 1 unless there is a specific reason why 2 is chosen.

ERROR = 1,
INFO = 2,
DEBUG = 3

index.ts
@@ -404,7 +416,7 @@ function readLastLine(path: fs.PathOrFileDescriptor) {

// I should really optimize or completely remove this line
if (config.logLines) {
console.log(lastLine);
log(LOGLEVELS.DEBUG, `Got message from Factorio ${lastLine}`);
Copy link

Choose a reason for hiding this comment

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

Colon after "Factorio"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikhailmikhalchuk mikhailmikhalchuk mikhailmikhalchuk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mattie112 @mikhailmikhalchuk @AGuyNamedJens