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: do not suggest incorrect form. #134

Closed ccoVeille closed 5 months ago

ccoVeille commented 5 months ago

I noticed something strange with the code suggested by formatter

I had to check and experiment a bit, but I think I found what was my problem

I was surprised the code was taking this

assert.Equal(t, 1, 2, fmt.Sprintf("foo %s", "bar"))

and produced this

assert.Equal(t, 1, 2, "foo %s", "bar")

At first, I thought the equal + sprintf form was invalid. Then I looked at the code of testify, and found it was OK.

But I was expecting this (so Equalf, not Equal)

assert.Equalf(t, 1, 2, "foo %s", "bar")

But I was surprised by the behavior anyway, and thus by the existence of Whatever vs Whateverf in testify

I asked for help on Gopher's Slack https://gophers.slack.com/archives/C18A3680H/p1718878075548759

And the reply was: someone reported it as invalid https://github.com/stretchr/testify/discussions/1560

I was amused to see you were the one quoted.

Then I looked at the code again, then I was expecting

assert.Equal(t, 1, 2, "foo %s", "bar")

to be replaced by this.

assert.Equalf(t, 1, 2, "foo %s", "bar")

Because this and especially this suggests formatter should convert it

Is there something I'm missing ? a parameter to pass ?

ccoVeille commented 5 months ago

I'm feel stupid

I found --formatter.require-f-funcs

Anyway, maybe it's a valid point

why would this

assert.Equal(t, 1, 2, fmt.Sprintf("foo %s", "bar"))

be replaced into this assert.Equal(t, 1, 2, "foo %s", "bar") and not assert.Equalf(t, 1, 2, "foo %s", "bar") no matter the value of --formatter.require-f-funcs

Antonboom commented 5 months ago

@ccoVeille please, stop spam issues

Every checker has detailed description in README. Formatter even has a historical reference – https://github.com/Antonboom/testifylint?tab=readme-ov-file#formatter

why would this be replaced into this ... no matter the value

Because it leads to inconsistency in tests code. If people are not aware about f-funcs (90%), then when you will replace assert with Equalf, but other asserts (without Sprintf) will still use Equal

assert.Equalf(t, 1, 2, "fixed assert %s", "bar")
assert.Equal(t, 1, 2, "neighbour valid assert %s", "bar")
pgimalac commented 3 months ago

please, stop spam issues

For the record I had the exact same thought process, I didn't know that assert.Equal and assert.Equalf behaved the same with respect to the format string (I read the whole README but missed the historical reference of formatter...), so this issue was pretty useful to me !

ccoVeille commented 3 months ago

Thanks

ccoVeille commented 1 month ago

177 was also reported for this issue.

And closed quickly, without debate then discussion was locked.

I feel this project is not open to discussion nor contributions.

When multiple people reports you something is strange, at some point you have to question yourself about the fact you might be wrong, or at least your project is not providing a solution for what people wants.

When you have a problem with everyone, or everyone as a problem, the problem is likely to be you

I came for contributing, I face too many times:

The fact you got a good idea with testifylint doesn't mean people are able to work with you.

I could have silent quited your project. But I'm not the kind to stay quiet when I see a bad behavior or someone who act as a bully or a tyrant.

I'm French, maybe it's cultural differences but it was a real pain to work with you @Antonboom

Especially because you behave as if we are working for you, not with you.

Thanks for the project, good luck for anyone who will try to work with you.

I'm not here to suffer.

If you have no idea what I could talk about in my messages, it means you have a lot of work on yourself to do.

Bye.

PS: @mmorel-35 it was a pleasure to work with you.