search

davecgh / go-spew

Implements a deep pretty printer for Go data structures to aid in debugging
ISC License
6.05k stars 364 forks source link

Use methods to sort keys, if so enabled. #31

Closed thockin closed 9 years ago

thockin commented 9 years ago

Review on Reviewable

thockin commented 9 years ago

Partially addresses #30

thockin commented 9 years ago

hmm, I just realized what I think is a gap in testing covering a bug, please hold :)

thockin commented 9 years ago

Re-pushed. This should be better.

thockin commented 9 years ago

I added a second commit for using spew itself to generate keys. Yell if you want me to do 2 PRs :)

davecgh commented 9 years ago

Thanks. I'll take a look at this pr this weekend. However I did notice the build failed due to decreased coverage. Please add tests which cover the new code.

EDIT: Well I see you did add tests. Perhaps this is due to something else. I'll take a look at when I review the PR.

thockin commented 9 years ago

Interesting. I thought I added good tests. I am going to be AFK most of the next 4 days, will take a look at tests when I can. Mean time, please offer any code-level feedback that would invalidate tests anyway :)

On Thu, Apr 2, 2015 at 9:22 AM, Dave Collins notifications@github.com wrote:

Thanks. I'll take a look at this pr this weekend. However I did notice the build failed due to decreased coverage. Please add tests which cover the new code.

— Reply to this email directly or view it on GitHub https://github.com/davecgh/go-spew/pull/31#issuecomment-88964939.

davecgh commented 9 years ago

@thockin: Yeah I edited my comment to say I see you did add tests. Perhaps this is due to something else. I'll take a look at when I review the PR.

thockin commented 9 years ago

I know why coverage went down - I had added a test for CanInterface() that (as far as I can tell) can never ever trigger. Removed

thockin commented 9 years ago

Just a gentle ping - I'd love to vendor this into our tree if you merge it :)

davecgh commented 9 years ago

@thockin Thanks for the nice work on this! I pointed out a few minor things, but overall I'm really pleased. All of the new code is tested, the documentation updates are solid, and the new code fairly closely matches the style used around it.

I apologize for the delay in reviewing. These last 2-3 weeks have been particularly grueling in terms of having almost no free time.

davecgh commented 9 years ago

Also, would you add the new config option to doc.go, so it shows up on GoDoc?

thockin commented 9 years ago

I understand grueling weeks :) This was my distraction from my own grueling weeks :) Thanks for the review.

I think all comments are addressed, and docs are updated (sorry for missing doc.go). Open issue: I have added a new commit with teh changes, for easiest review. I can patch this into the two individual commits if super-clean history matters to you (sort of a PITA but I'll do it) or you can take it as it is, or I can squash it all to a single commit.

thockin commented 9 years ago

I blame the wrist brace I have to wear :) Typo fixed

davecgh commented 9 years ago

Squashing it to a single commit is great. In fact, I was going to to ask for that after I finished the reviewing the latest changes.

davecgh commented 9 years ago

Looks good now! Thanks a lot for your work on this. I'll merge it after the squash.

thockin commented 9 years ago

squashed, commit message clarified to reflect both changes.