Removing nested trace output from trace.go #168

krisnova · 2017-02-03T03:25:14Z

My first attempt at a PR..

Closes #129

@sdboyer when you get a chance let me know what you think!

krisnova · 2017-02-03T03:28:35Z

Sample output with Kubernetes kops and example.go

glide install && go build -o gps example.go && chmod +x gps && mv gps ~/Workspace/kops && cd ~/Workspace/kops/ && ./gps && rm -rf .repocache; cd -

Output here

codecov · 2017-02-03T03:58:41Z

Codecov Report

Merging #168 into master will increase coverage by -0.06%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   79.34%   79.28%   -0.06%     
==========================================
  Files          24       24              
  Lines        3698     3707       +9     
==========================================
+ Hits         2934     2939       +5     
- Misses        567      570       +3     
- Partials      197      198       +1

Impacted Files	Coverage Δ
trace.go	`71.69% <75%> (-1.5%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fe10c6...3cd8278. Read the comment docs.

codecov · 2017-02-03T03:58:41Z

Codecov Report

Merging #168 into master will increase coverage by -0.29%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   79.34%   79.05%   -0.29%     
==========================================
  Files          24       24              
  Lines        3698     3709      +11     
==========================================
- Hits         2934     2932       -2     
- Misses        567      577      +10     
- Partials      197      200       +3

Impacted Files	Coverage Δ
trace.go	`72.22% <81.48%> (-0.98%)`	❌
source_errors.go	`0% <ø> (-50%)`	❌
source.go	`42.69% <ø> (-2.93%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fe10c6...5182d7d. Read the comment docs.

sdboyer · 2017-02-03T04:41:47Z

Huh. Well. Simplest really is best - I think I might like that more than any of the other options I discussed in #129 :)

One thing, though - I think it might still be valuable to indent the "inner" rows - things not starting with "select", "attempt", "revisit" or "backtrack". Yes, you can infer it from the counter on the left, but that's not the most useful for visual scanning and grouping. I'd at least like to see if doing it that way provides an improved experience.

Also, that leading pipe feels unnecessary now - maybe we can drop that, too.

sdboyer · 2017-02-03T04:42:41Z

trace.go

+func getprei(i int) string {
+	var s string
+	if i < 10 {
+		s = fmt.Sprintf("(%d)   ", i)


At this point, might it be safer/better to just use a tab?

1. Remove pipe 2. Indent inner messages over 2 spaces 3. s/3spaces/tab in getprei

krisnova · 2017-02-03T05:11:59Z

@sdboyer Pushed up changes per your review.. Totally agree about the pipe

I think I got all of the "inner" messages - but if you could check that would be helpful

Also I indented the "inner" messages over 2 spaces.. I tried a few options, and that looked the best.

Sample output:

Root project is "/Users/kris/Workspace/kops"
 69 transitively valid internal packages
 132 external packages imported from 17 projects
(0)   ✓ select (root)
(1)   ? attempt github.com/ghodss/yaml with 1 pkgs; 1 versions to try
(1)       try github.com/ghodss/yaml@master
(1)   ✓ select github.com/ghodss/yaml@master w/1 pkgs
(2)   ? attempt github.com/sergi/go-diff with 1 pkgs; 1 versions to try
(2)       try github.com/sergi/go-diff@master
(2)   ✓ select github.com/sergi/go-diff@master w/1 pkgs
(3)   ? attempt github.com/golang/glog with 1 pkgs; 1 versions to try
(3)       try github.com/golang/glog@master
(3)   ✓ select github.com/golang/glog@master w/1 pkgs
(4)   ? attempt github.com/spf13/viper with 1 pkgs; 1 versions to try
(4)       try github.com/spf13/viper@master
(4)   ✓ select github.com/spf13/viper@master w/1 pkgs
(5)   ? attempt github.com/mitchellh/mapstructure with 1 pkgs; 1 versions to try
(5)       try github.com/mitchellh/mapstructure@master
(5)   ✓ select github.com/mitchellh/mapstructure@master w/1 pkgs
(6)   ? attempt github.com/spf13/afero with 1 pkgs; 1 versions to try
(6)       try github.com/spf13/afero@master
(6)   ✓ select github.com/spf13/afero@master w/1 pkgs
(7)   ? attempt github.com/spf13/cobra with 2 pkgs; 1 versions to try
(7)       try github.com/spf13/cobra@master
(7)   ✓ select github.com/spf13/cobra@master w/2 pkgs
(8)   ? attempt github.com/inconshreveable/mousetrap with 1 pkgs; 1 versions to try
(8)       try github.com/inconshreveable/mousetrap@master
(8)   ✓ select github.com/inconshreveable/mousetrap@master w/1 pkgs
(9)   ? attempt golang.org/x/text with 2 pkgs; 1 versions to try
(9)       try golang.org/x/text@master

sdboyer · 2017-02-03T05:42:05Z

trace.go

 	default:
 		// panic here because this can *only* mean a stupid internal bug
-		panic(fmt.Sprintf("canary - unknown type passed as first param to traceInfo %T", data))
+		panic(fmt.Sprintf("%scanary - unknown type passed as first param to traceInfo %T", innerIndent, data))


nit: no need to indent on the panic

Ah good catch - fixed 😄

still have a % :P

SMH - Should I squash?

sdboyer · 2017-02-03T06:09:27Z

Yep, we have a winner! 🎉

Removing nested trace output from trace.go

Adding initial commit for trace output

2676f05

Go fmt

3cd8278

sdboyer reviewed Feb 3, 2017

View reviewed changes

Changes per review

9c655bb

1. Remove pipe 2. Indent inner messages over 2 spaces 3. s/3spaces/tab in getprei

sdboyer reviewed Feb 3, 2017

View reviewed changes

krisnova added 2 commits February 2, 2017 22:45

Remove indent on panic

ed37fd4

Removing extraneous %

5182d7d

sdboyer merged commit a04c90d into sdboyer:master Feb 3, 2017

krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017

Merge pull request sdboyer/gps#168 from kris-nova/viz-improvements

c79c72d

Removing nested trace output from trace.go

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing nested trace output from trace.go #168

Removing nested trace output from trace.go #168

krisnova commented Feb 3, 2017

krisnova commented Feb 3, 2017

codecov bot commented Feb 3, 2017

codecov bot commented Feb 3, 2017 •

edited

Loading

sdboyer commented Feb 3, 2017

sdboyer Feb 3, 2017

krisnova commented Feb 3, 2017 •

edited

Loading

sdboyer Feb 3, 2017

krisnova Feb 3, 2017

sdboyer Feb 3, 2017

krisnova Feb 3, 2017

sdboyer commented Feb 3, 2017

Removing nested trace output from trace.go #168

Removing nested trace output from trace.go #168

Conversation

krisnova commented Feb 3, 2017

krisnova commented Feb 3, 2017

Sample output with Kubernetes kops and example.go

codecov bot commented Feb 3, 2017

Codecov Report

codecov bot commented Feb 3, 2017 • edited Loading

Codecov Report

sdboyer commented Feb 3, 2017

sdboyer Feb 3, 2017

Choose a reason for hiding this comment

krisnova commented Feb 3, 2017 • edited Loading

Sample output:

sdboyer Feb 3, 2017

Choose a reason for hiding this comment

krisnova Feb 3, 2017

Choose a reason for hiding this comment

sdboyer Feb 3, 2017

Choose a reason for hiding this comment

krisnova Feb 3, 2017

Choose a reason for hiding this comment

sdboyer commented Feb 3, 2017

codecov bot commented Feb 3, 2017 •

edited

Loading

krisnova commented Feb 3, 2017 •

edited

Loading