Remove the default console logger when it is not set in the configuration #602
Conversation
| @@ -867,6 +867,16 @@ func newLogService() { | |||
| LogModes = strings.Split(Cfg.Section("log").Key("MODE").MustString("console"), ",") | |||
| LogConfigs = make([]string, len(LogModes)) | |||
|
|
|||
| useConsole := false; | |||
| for _, mode := range LogModes { | |||
| if "console" == mode { | |||
There was a problem hiding this comment.
is this "safer" or just a code style ? in Java (yes that is why there are ; every where 😄 ) the "console".equals(mode) is null safe
still followed your proposal
There was a problem hiding this comment.
You can't set variable as a value in if condition.
package main
import "fmt"
func main() {
a := 1
if a = 1 {
fmt.Println("a=1")
}
}It is not readable for value == variable
| @@ -867,6 +867,16 @@ func newLogService() { | |||
| LogModes = strings.Split(Cfg.Section("log").Key("MODE").MustString("console"), ",") | |||
| LogConfigs = make([]string, len(LogModes)) | |||
|
|
|||
| useConsole := false; | |||
| useConsole := false; | ||
| for _, mode := range LogModes { | ||
| if "console" == mode { | ||
| useConsole = true; |
| } | ||
| if (!useConsole) { | ||
| log.DelLogger("console"); | ||
| } |
There was a problem hiding this comment.
if !useConsole {
log.DelLogger("console")
}
|
Have you tried that it works, both with and without console enabled? :) |
tboerger
added
the
lgtm/need 2
| return l.DelLogger(mode) | ||
| } | ||
| } | ||
| panic("log: unknown adapter \"" + mode + "\" (forgotten register?)") |
There was a problem hiding this comment.
This is slightly cleaner :)
panic(`log: unknown adapter "` + mode + `" (forgotten register?)`)
|
yes tested it with and without console enabled |
| } | ||
| } | ||
| if (!useConsole) { | ||
| log.DelLogger("console") |
There was a problem hiding this comment.
With this implementation, we always add the console logger (setting.go: line 421), and then sometimes remove it later (this if statement). Would it make more sense to check whether we need the console logger when we initially add it, instead of checking after we've already added it?
There was a problem hiding this comment.
Yes that is correct, was thinking about setting the log level earlier but then you could miss some information about where the custom log file is read from
So that is why I have decided to do it this way
…nstead of adapter, otherwise panic when reinstalling gitea (since the output adapter still exist, without outputs)
|
LGTM for now |
tboerger
added
lgtm/need 1
|
LGTM |
tboerger
added
lgtm/done
|
@willemvd This could be back port to v1.0. |
…tion (go-gitea#602) * Remove the default console logger when it is not set in the configuration * Added comment to new function (lint failure) * update based on PR comments (code style) * code style fix (thanks bkcsoft) * check if logger exists based on the l.outputs (like in l.DelLogger) instead of adapter, otherwise panic when reinstalling gitea (since the output adapter still exist, without outputs)
…tion (#602) (#960) * Remove the default console logger when it is not set in the configuration * Added comment to new function (lint failure) * update based on PR comments (code style) * code style fix (thanks bkcsoft) * check if logger exists based on the l.outputs (like in l.DelLogger) instead of adapter, otherwise panic when reinstalling gitea (since the output adapter still exist, without outputs)
Fix for issue #581
Removes the default console logger when it is not set in the configuration
Mark: this is my first Go programming experience so please be gentle ;)