Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

log.Logger api changes #1617

Merged
merged 2 commits into from
Feb 8, 2018
Merged

log.Logger api changes #1617

merged 2 commits into from
Feb 8, 2018

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented Feb 2, 2018

What does this do / why do we need it?

This PR proposes making some exported API surrounding logging more flexible. The effect is simpler caller code (e.g. nil rather than discardLogger), and completely removing some "log" imports from gps.

gps:

  • PruneProject: remove unused logger
  • WriteDepTree: swap logger for onWrite callback
  • fix example

dep:

  • SafeWriter.Write: nilable logger

cmd:

  • ensure/prune/init: react to changes

Do you need help or clarification on anything?

Is there a better name for gps.WriteProgress?

Which issue(s) does this PR fix?

Towards #672.

gps:
	-PruneProject: remove unused logger
	-WriteDepTree: swap logger for onWrite callback
	-fix example

dep:
	-SafeWriter.Write: nilable logger

cmd:
	-ensure/prune/init: react to changes
msg := "Wrote"
if err != nil {
msg = "Failed to write"
if onWrite != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No more locking to serialize to ioutil.Discard

func WriteDepTree(basedir string, l Lock, sm SourceManager, co CascadingPruneOptions, logger *log.Logger) error {
//
// If onWrite is not nil, it will be called after each project write. Calls are ordered and atomic.
func WriteDepTree(basedir string, l Lock, sm SourceManager, co CascadingPruneOptions, onWrite func(WriteProgress)) error {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rather than log or nothing, callers get a rich type they can inspect and which also knows how to log itself.

Copy link
Collaborator

@ibrasho ibrasho left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

LGTM

@jmank88 jmank88 merged commit a45e03b into golang:master Feb 8, 2018
@jmank88 jmank88 deleted the log_api branch February 8, 2018 13:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants