Antonboom / testifylint

The Golang linter that checks usage of github.com/stretchr/testify.
https://github.com/stretchr/testify
MIT License
101 stars 8 forks source link

formatter: remove empty message string in assert.Equalf conversions (autofix) #194

Open ghouscht opened 1 month ago

ghouscht commented 1 month ago

Hey, While working on enabling testifylint, specifically the formatter (https://github.com/etcd-io/etcd/pull/18741) we found a potential improvement for the automatic fix in the formatter check with require-f-funcs option enabled.

In the codebase we found several occasions of assertions like this:

assert.Equal(t, want, got, "")

testifylint autofix automatically replaced them with that: (assert.Equalf)

assert.Equalf(t, want, got, "")

which is fine but I belive it would be better to remove the empty message string and replace as follows:

assert.Equal(t, want, got)

What do you think about that? Would you accept a PR that changes this behaviour?

Antonboom commented 1 week ago

@ghouscht hi!

thank you for issue

these are really strange occasions :D

What do you think about that?

I am ok this that.

Would you accept a PR that changes this behaviour?

Yes, appreciated.

But need to introduce new messages for this

formatter: empty format string
formatter: empty string among formatted arguments

And to test it carefully:

  1. assert.Equal(t, want, got, "") -> assert.Equal(t, want, got)
  2. assert.Equalf(t, want, got, "") -> assert.Equal(t, want, got)
  3. assert.Equal(t, want, got, "msg %d", "") -> no autofix, warn on empty string in args
  4. assert.Equal(t, want, got, "msg", "") -> no autofix, warn on empty string in args
  5. assert.Equal(t, want, got, "msg %d %v", "", "3") -> no autofix, warn on empty string in args
  6. maybe some Sprintf cases? please, look at current formatter tests and grep some ideas

but 3-5 looks like issue in go vet's printf, that could be natively inherited by testifylint without extra dev on our side.