-
Notifications
You must be signed in to change notification settings - Fork 1k
Add verbose logging option to WriteDepTree #968
Conversation
This is just a heads up that #952 is moving this method into gps and tweaking parameters as well. The race to merge is on! 😀 |
internal/gps/result.go
Outdated
if l == nil { | ||
return fmt.Errorf("must provide non-nil Lock to WriteDepTree") | ||
} | ||
|
||
if logger != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little concerned that by using a nil logger, instead of a discard logger, that we may end up with nil pointer exceptions down the road as these functions evolve are worked on by new people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should i define a logger interface? If so should i change other functions taking *log.Logger
to accept that interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, i can use log.New(ioutil.Discard, "", 0)
, sorry no need for interface 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to define ctx.VerboseLogger
instead of Verbose
bool. Verbose
flag is mostly used as
if verbose{
logger.Print ....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I am not really sure. We haven't settled on a standard yet, but what you see on ctx is what we have been doing so far.
I suggest that for now, follow the other examples in dep for verbose logging. If after you've done that, you see a way to simplify verbose logging, let's look at that in a separate issue or PR.
No need to close the PR. I was just letting you know that you may need to rebase and incorporate changes from the other, depending on who merges first. 😁 As for #534, yes this may help with that (which would be awesome!) but I suggest limiting this PR to just the verbose logging, so that it's easier to review. |
Is it ok to send verbose logs to stdout or should i use stderr. And if nil logger will not be used i can also change prune commands behavior as well. |
Verbose logs should be written to Let's stick to only updating |
If this seems ok, i can squash commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just one small change, otherwise this looks ready to merge.
Originally I wanted to ask that we add a test that the verbose output for each command is printed as expected, but our test harness doesn't make that easy, and unit tests for that wouldn't quite capture what I'd like to test. I'll open another issue to discuss how we could tweak the test harness to validate verbose output for the commands.
@@ -195,6 +196,15 @@ func (lp LockedProject) Packages() []string { | |||
return lp.pkgs | |||
} | |||
|
|||
func (lp LockedProject) String() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this has been extracted into a function, would you mind adding a small unit test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sure
Don't worry about squashing, I can do that when I merge. |
internal/gps/lock.go
Outdated
@@ -195,6 +196,15 @@ func (lp LockedProject) Packages() []string { | |||
return lp.pkgs | |||
} | |||
|
|||
func (lp LockedProject) String() string { | |||
prj := lp.Ident() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for consistency with other references in gps, please name this var id
internal/gps/lock.go
Outdated
if prj.Source == "" { | ||
return fmt.Sprintf("%s@%s", prj.ProjectRoot, rev) | ||
} | ||
return fmt.Sprintf("%s -> %s@%s", prj.ProjectRoot, prj.Source, rev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a method that does the equivalent of this already - ProjectIdentifier.errString()
- but it represents the same information in a different way. let's reuse that method here, instead of creating a new visual representation of the notion of an alternate source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that function (and this review 😬 i will fix this) ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem, there's a lot in gps - easy to miss things!
internal/gps/result.go
Outdated
if l == nil { | ||
return fmt.Errorf("must provide non-nil Lock to WriteDepTree") | ||
} | ||
|
||
logger.Println("Creating vendor dir ...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think this message adds much value. for one, this func isn't actually guaranteed to be creating a vendor/
dir - the way we use it right now, it's actually writing to a temporary directory, which will later be swapped into place as a vendor/
dir, assuming no errors occur.
if dep wants to put some kind of header message like this in place from within SafeWriter.Write()
, that's fine, but let's keep what's printed directly here to a minimum.
internal/gps/lock.go
Outdated
@@ -195,6 +196,15 @@ func (lp LockedProject) Packages() []string { | |||
return lp.pkgs | |||
} | |||
|
|||
func (lp LockedProject) String() string { | |||
prj := lp.Ident() | |||
rev, _, _ := VersionComponentStrings(lp.Version()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm...i think that only showing the revision here is probably not the optimal UX. it'd be better to show the full paired information - version or branch, and then rev in parens. it can follow the same pattern as in GetLockingFeedback()
. the logic will have to be duplicated, though - not only do we try very hard to encapsulate gps into having no knowledge of dep, but that would create an import cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Version
defines Stringer
instead of VersionComponentStrings
, I directly used it, should i edit versionPairs
Stringer definition to include both version and rev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's leave versionPair.String()
out of this, at least for now, and just accept a bit of duplicate code for this particular case.
internal/gps/result.go
Outdated
@@ -65,6 +68,7 @@ func WriteDepTree(basedir string, l Lock, sm SourceManager, sv bool) error { | |||
for _, p := range l.Projects() { | |||
to := filepath.FromSlash(filepath.Join(basedir, string(p.Ident().ProjectRoot))) | |||
|
|||
logger.Printf("Ensure %v exists", p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that using "ensure" here may blur the meaning of the word a bit. let's say "Writing out %s"
here, instead.
From a quick glance, no comments from my side. Also, I don't think conflicts with #952 are going to be a problem now. 👍 |
internal/gps/lock_test.go
Outdated
want string | ||
}{ | ||
{ | ||
name: "Full Info", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: either all lower case ("full info") or normal sentence case ("Full info") for all of these, to be maximally consistent with our other subtest naming.
internal/gps/lock.go
Outdated
@@ -195,6 +196,11 @@ func (lp LockedProject) Packages() []string { | |||
return lp.pkgs | |||
} | |||
|
|||
func (lp LockedProject) String() string { | |||
return fmt.Sprintf("%s@%v for packages: %v", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. so, we've a pair of problems.
first is that this is a generic Stringer
implementation, but word choice is oriented towards the one use case we currently have: using the preposition "for" makes the most sense as a sort of explanation "we have this project for packages X, Y, Z within it." but that seems less than ideal, in part because that list of packages includes ones that are only required internally (e.g., some other project imports root/A
, which imports its sibling root/B
; both show up in the packages
list.) to my mind, "with" is a bit better than "for", because it's more of a plain statement/less expository.
however, there's also the problem that listing out the packages at all probably gives the wrong impression to the user - it suggests that we're writing out only those packages, when we're actually writing out the entire tree.
so, let's use an unexported, purpose-specific func for generating this string, and omit package information from it entirely. basically, just the "%s@%v"
part.
however, since we've already been through all this with the LockedProject.String()
method, i'm ok with leaving it in (even though we're not using it), as long as we s/for/with/.
are you good with this, @carolynvs? if so, approve and then feel free to merge, this LGTM 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thank you! ❤️
What does this do / why do we need it?
Add logging parameter to
gps.WriteDepTree
andsw.Write
to print each step if verbose option is selected.What should your reviewer look out for in this PR?
sw.Write
andgps.WriteDepTree
sw.PrintPreparedActions
andgps.WriteDepTree
logs LockedProject to user.Do you need help or clarification on anything?
I implemented as we speak in #963. It might be better to implement this functionality inside SourceManager.
Which issue(s) does this PR fix?
fixes #963