Skip to content

Conversation

rwcarlsen
Copy link
Contributor

This includes changes submitted in pull request #1. My additions:

  • assertSeqEqual was broken up into 3 type-safe, simpler alternatives that use the more informative testing.Fatal methods.
  • failing Diff assertions now print the line of the actual problem along with some pretty diff info.
  • fixed all failing tests
  • added a New method to replace the private createDMP to make the package a bit nicer to use and more Go idiomatic.
  • DiffMain now is a wrapper around diffMain and takes no interface{} args, and 1 bool (checklines) arg (only backwards-incompatible change).
  • other misc tweaks.

@rwcarlsen
Copy link
Contributor Author

@sergi Is this still being actively maintained/worked on?

@sergi
Copy link
Owner

sergi commented Mar 13, 2013

Hi @rwcarlsen,

Yes, it is still being maintained. I am going through your changes, they look great. I will merge them as soon as possible.

Sergi

@sergi
Copy link
Owner

sergi commented Mar 13, 2013

I am getting the following after go test in your refactor branch:

➜  diff git:(master) go test
--- FAIL: Test_diffMain (0.26 seconds)
        Location:       dmp_test.go:904
    Error:      Should be true
    Messages:   252.795ms !< 200ms

FAIL
exit status 1
FAIL    _/Users/sergi/programming/diff_refactor/diff    0.280s

Do you get the same?

@rwcarlsen
Copy link
Contributor Author

This is for the timeout test - and it passes on my computer. I suspect it has to do with the fact that the diff algorithm does lots of things in between timeout checks - and my computer may be a bit speedier than yours. We might want to change the assert to be x * dmp.DiffTimeout where x is 3 or 4 instead of 2 (line 904 in dmp_test). Also - it looks like I forgot to reduce the number of string doublings (loop iters) back to the original 10 for that test.

@sergi
Copy link
Owner

sergi commented Mar 13, 2013

I see. In that case, it looks good to me, and I will change that timeout after merging your refactor. Thank you so much for this pull request, I learned a lot of idiomatic Go from it, and it improves my newbie lib code a lot!

sergi pushed a commit that referenced this pull request Mar 13, 2013
Fixed all of the tests/bugs.  Test error output improvements. Minor API change.
@sergi sergi merged commit 3a304e6 into sergi:master Mar 13, 2013
@rwcarlsen rwcarlsen mentioned this pull request Mar 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants