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

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

Open
ghouscht opened this issue Oct 16, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@ghouscht
Copy link

Hey,
While working on enabling testifylint, specifically the formatter (etcd-io/etcd#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
Copy link
Owner

Antonboom commented Nov 13, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants