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

move Logger into Ctx; restore logging #544

Merged
merged 1 commit into from
May 12, 2017
Merged

move Logger into Ctx; restore logging #544

merged 1 commit into from
May 12, 2017

Conversation

jmank88
Copy link
Collaborator

@jmank88 jmank88 commented May 10, 2017

This change:

  • moves the Loggers definition alongside Ctx
  • adds a Loggers field to Ctx
  • restores warning logging

Fixes #531

@darkowlzz
Copy link
Collaborator

oops! #545 😅

@sdboyer sdboyer mentioned this pull request May 11, 2017
@sdboyer
Copy link
Member

sdboyer commented May 11, 2017

I was hasty and incorrect when laying out the desired goal here in that FIXME. Sorry, I need to backtrack a bit.

We shouldn't be logging anything from within readManifest(); that decision needs to be made at a higher level.

  1. Instead, if any warnings are encountered, we should pass those out as well, via a third return value (so the sig becomes func readManifest(r io.Reader) (*Manifest, []error, error)).
  2. When calling readManifest() from Analyzer.DeriveManifestAndLock(), for the moment, we should ignore the new []error return value.
  3. When calling readManifest() from Ctx.LoadProject(), if there are warnings returned, we should immediately log them to stderr, but continue.

The basic idea here is that, when the Analyzer is reading a manifest, it's doing so for a dependency on behalf of the SourceManager. It's not examining the current project. In that case, we want to be lenient, as those errors are in other projects' Gopkg.toml - something the user is unlikely to be able to do anything about. Showing non-actionable errors to the user is generally a mistake.

When Ctx is reading Gopkg.toml, though, it's doing it for the current project. The user IS empowered to do something about that. So we show the errors.

I'm tempted to make #3 even stronger and just immediately exit nonzero with an appropriate general error - e.g. "Invalid values found in Gopkg.toml". But that's quite draconian, and out of keeping with how most tools treat their config files. Plus, it would make it very awkward for us to add new fields to Gopkg.toml in the future, which is something we definitely want to do. So we'll have to accept just warnings.

@jmank88
Copy link
Collaborator Author

jmank88 commented May 11, 2017

we should the new []error return value.

I think you accidentally a word. Though the following paragraph suggests 'ignore', is that correct?

@sdboyer
Copy link
Member

sdboyer commented May 11, 2017

I think you accidentally a word. Though the following paragraph suggests 'ignore', is that correct?

And such a crucial word, lol - yes, "ignore" was what I dropped. (editing now)

@sdboyer
Copy link
Member

sdboyer commented May 12, 2017

brilliant - thank you!

@sdboyer sdboyer merged commit 4e09d39 into golang:master May 12, 2017
@jmank88 jmank88 deleted the restore_logging branch May 12, 2017 12:01
This pull request was closed.
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