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

Remove all global log invocations #31

Merged
merged 1 commit into from
Mar 24, 2016
Merged

Conversation

peterbourgon
Copy link
Contributor

Fixes #21

@@ -30,15 +30,17 @@ type remoteConnection struct {
remoteTCPAddr string
outbound bool
established bool
logger *log.Logger

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Mar 23, 2016

Is putting a logger field into most structs, and adding an extra arg to the initialisation methods really the best way to go about this?

Most things have access to router.Ourself, so could just use the logger in that.

Though ironically with my changes in #33 and the changes in here, I think LocalPeer isn't actually doing any logging itself anymore - it only needs a logger in order to feed it into the LocalConnection constructor.

@peterbourgon
Copy link
Contributor Author

I elected to make the minimum changes necessary. I am happy to refactor further. May I delete methods like remoteConnection.log, or is that prefix useful/necessary?

@rade
Copy link
Member

rade commented Mar 24, 2016

#33 gets rid of remoteConnection.log.

@@ -22,7 +24,7 @@ var _ gossipConnection = &mockGossipConnection{}

func newTestRouter(name string) *Router {
peerName, _ := PeerNameFromString(name)
router := NewRouter(Config{}, peerName, "nick", nil)
router := NewRouter(Config{}, peerName, "nick", nil, log.New(ioutil.Discard, "", 0))

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@peterbourgon peterbourgon merged commit f4a38fe into master Mar 24, 2016
@peterbourgon peterbourgon deleted the 21-no-global-logger branch March 24, 2016 12:17
@bboreham
Copy link
Contributor

bboreham commented Apr 4, 2016

It would be easier to integrate this with other schemes if it took an interface rather than a concrete *log.Logger.

@peterbourgon
Copy link
Contributor Author

Agreed. #37

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.

3 participants