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 method for setting default file or writer #25

Merged
merged 8 commits into from Dec 19, 2017
Merged

Add method for setting default file or writer #25

merged 8 commits into from Dec 19, 2017

Conversation

RadhiFadlillah
Copy link
Contributor

The main focus of this PR is to add new method for setting default output file if there are no output defined for some or all log level. This method is especially useful if you want to put all log level to single file.

package main

import (
	"github.com/RadhiFadlillah/lfshook"
	"github.com/sirupsen/logrus"
)

func main() {
	// Create lfs hook
	hook := lfshook.NewHook(nil, nil)
	hook.SetDefaultPath("general.log")

	// Connect hook with logrus
	logrus.StandardLogger().AddHook(hook)

	// Create some log
	logrus.Infoln("This is info")
	logrus.Warnln("This is warning")
	logrus.Errorln("This is error")
	logrus.Fatalln("This is fatal")
}

Code above will make a general.log file which contains all logs :

time="2017-12-10T22:11:22+07:00" level=info msg="This is info"
time="2017-12-10T22:11:22+07:00" level=warning msg="This is warning"
time="2017-12-10T22:11:22+07:00" level=error msg="This is error"
time="2017-12-10T22:11:22+07:00" level=fatal msg="This is fatal"

While adding this method, I fixed some bugs :

  1. lfshook will fail to create new log file if target directory doesn't exist.
  2. The formatter that pased through constructor won't disable color mode in TextFormatter, which make log files looks weird.

Copy link
Owner

@rifflock rifflock left a comment

Choose a reason for hiding this comment

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

This definitely makes it cleaner than having to create a map of identical paths/writers.

Ideally we'd be able to set a default path AND a path map (or writer and writer map) in the constructor, but I'd hate to change the constructor signature again.

lfshook.go Outdated
switch levelMap.(type) {
case nil:
break
Copy link
Owner

Choose a reason for hiding this comment

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

If we go this path it might be good to instead support a case for string (default path) and a case for io.Writer (default writer).

I'm not a big fan of allowing the user to successful create a hook that has no valid output paths configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done on 83da246.

@rifflock
Copy link
Owner

rifflock commented Dec 12, 2017

I tried to resolve the merge conflicts to the Readme, but it wouldn't let me (probably because it's your branch). If you can fix those then I can merge it.

@RadhiFadlillah
Copy link
Contributor Author

@rifflock done.

Copy link
Owner

@rifflock rifflock left a comment

Choose a reason for hiding this comment

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

This looks good. Should be a non-breaking change.

@rifflock rifflock merged commit 1fdc019 into rifflock:master Dec 19, 2017
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.

None yet

2 participants