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

Remove the default console logger when it is not set in the configuration #602

Merged
merged 5 commits into from
Jan 17, 2017
Merged

Remove the default console logger when it is not set in the configuration #602

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb09b63
Remove the default console logger when it is not set in the configura…
willemvd Jan 6, 2017
3749eaf
Added comment to new function (lint failure)
willemvd Jan 6, 2017
e22626d
update based on PR comments (code style)
willemvd Jan 6, 2017
b6ca28d
code style fix (thanks bkcsoft)
willemvd Jan 6, 2017
c39918c
check if logger exists based on the l.outputs (like in l.DelLogger) i…
willemvd Jan 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions modules/log/log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ func NewLogger(bufLen int64, mode, config string) {
}
}

// DelLogger removes loggers that are for the given mode
func DelLogger(mode string) error {
for _, l := range loggers {
if l.adapter == mode {
return l.DelLogger(mode)
}
}
panic("log: unknown adapter \"" + mode + "\" (forgotten register?)")
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 slightly cleaner :)

panic(`log: unknown adapter "` + mode + `" (forgotten register?)`)

}

// NewGitLogger create a logger for git
// FIXME: use same log level as other loggers.
func NewGitLogger(logPath string) {
Expand Down
10 changes: 10 additions & 0 deletions modules/setting/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 mode == "console" {
useConsole = true
}
}
if (!useConsole) {
log.DelLogger("console")
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
Copy link
Member

Choose a reason for hiding this comment

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

if !useConsole {
  log.DelLogger("console")
}


for i, mode := range LogModes {
mode = strings.TrimSpace(mode)

Expand Down