Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove testify #872

Open
pelletier opened this issue May 19, 2023 · 5 comments
Open

Remove testify #872

pelletier opened this issue May 19, 2023 · 5 comments
Labels
good first issue Issues that are suitable for first-time contributors.

Comments

@pelletier
Copy link
Owner

pelletier commented May 19, 2023

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 pelletier added the good first issue Issues that are suitable for first-time contributors. label May 19, 2023
@ramisback
Copy link

Hi, I would like to take this issue

@pelletier
Copy link
Owner Author

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

@sparr
Copy link

sparr commented Jun 17, 2023

@ramisback Are you still planning to work on this?

@jidicula
Copy link
Contributor

jidicula commented Oct 2, 2023

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?

@pelletier
Copy link
Owner Author

pelletier commented Oct 2, 2023

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

4 participants