go-toml icon indicating copy to clipboard operation
go-toml copied to clipboard

Remove testify

Open pelletier opened this issue 1 year ago • 5 comments

The only dependency of go-toml v2 is stretchr/testify, which itself imports a bunch of stuff. I'd like this library to be dependence-free. The test suite only uses a few functions from testify, so it should be reasonably straightforward to implement them inside go-toml and replace the import statements.

Maybe they can be stored in /internal/require and /internal/assert to preserve the same layout.

pelletier avatar May 19 '23 14:05 pelletier

Hi, I would like to take this issue

ramisback avatar May 22 '23 03:05 ramisback

I'd be happy to review a pull request :)

pelletier avatar May 23 '23 22:05 pelletier

@ramisback Are you still planning to work on this?

sparr avatar Jun 17 '23 17:06 sparr

Hey @pelletier 👋

I can see a couple of approaches - either (manually) vendorize assert and require methods used in go-toml v2 as well as their dependencies into internal, or reimplement just their behaviour in internal/assert and internal/require. Do you have a preference on which might be better?

jidicula avatar Oct 02 '23 20:10 jidicula

Hi! I think it would be better to re-implement the minimal subset needed instead of vendoring (which would also bring the dependencies).

Looking at the current use of testify in this project:

~/s/g/p/go-toml v2$ ag -o --nofilename --nobreak 'require\.([a-zA-Z]+)'|sort|uniq -c
  83 require.Equal
  57 require.Error
   3 require.IsType
   1 require.LessOrEqual
   1 require.Nil
 129 require.NoError
   2 require.Panics
~/s/g/p/go-toml v2$ ag -o --nofilename --nobreak 'assert\.([a-zA-Z]+)'|sort|uniq -c
   1 assert.Contains
   7 assert.Empty
  55 assert.Equal
   9 assert.Error
   2 assert.InDelta
   3 assert.NoError
   2 assert.NotEmpty
   3 assert.True

I'd follow something like this.

First, pick one of assert or require and map one onto the other (for example, replace all require.* to assert.*). That should be reasonably easy with a search+replace. I don't have a strong preference between the two behaviors. Maybe assert is a bit nicer since more tests can continue until something panics. This should result in approximately:

~/s/g/p/go-toml v2$ ag -o --nofilename --nobreak '(require|assert)\.([a-zA-Z]+)'|sed s/require/assert/|sort|uniq -c|sort --reverse
 138 assert.Equal
 132 assert.NoError
  66 assert.Error
   7 assert.Empty
   3 assert.True
   3 assert.IsType
   2 assert.Panics
   2 assert.NotEmpty
   2 assert.InDelta
   1 assert.Nil
   1 assert.LessOrEqual
   1 assert.Contains

Which is only 12 functions, most of them straightforward to write, or lightly adapted from testify. Then once all the testify calls are all mapped, create an internal/testing or internal/assert package, and implement those functions, and finally remove the imports. It's probably worth looking into not having dedicated functions for those that are only used < 10 times!

I believe the hard part will be printing the diff when two values differ. Though I think a simple fmt.Printf("%+v") of the expected and the actual values is probably good enough to get started. But feel free to do something more fancy with reflect :)

pelletier avatar Oct 02 '23 21:10 pelletier