Closed ccoVeille closed 2 months ago
I've read the consideration about the doubt about using Zero vs Empty
I'm not sure if anyone uses assert.Zero – it looks strange and conflicts with assert.Empty: Maybe it's better to make configurable support for other types in the empty checker and vice versa to prohibit the Zero? Source: https://github.com/Antonboom/testifylint/blob/master/CONTRIBUTING.md#zero
I'll explain in this issue what I plan to implement, so we could talk about the implementation here
I created the issue to keep a track of it, and allow other people to discuss it.
Stay tuned, and ping me if you see no progress in the next month.
I've read the consideration about the doubt about using Zero vs Empty
I'm not sure if anyone uses assert.Zero – it looks strange and conflicts with assert.Empty: Maybe it's better to make configurable support for other types in the empty checker and vice versa to prohibit the Zero? Source: https://github.com/Antonboom/testifylint/blob/master/CONTRIBUTING.md#zero
I'll explain in this issue what I plan to implement, so we could talk about the implementation here
I agree with you @Antonboom the use case assert.Zero
covers are close to the one assert.Empty
I will check a bit further in the code of testify, but I think is what we should look for
https://github.com/stretchr/testify/blob/master/assert/assertions_test.go#L23-L85
I would say Zero
checker should report to be used when using Equal
, Equalf
, Exactly
and Exactlyf
for the following type
And same for NotZero
with NotEqual
and, NotEqualf
Everything other types that assert.Zero
supports should be reported to use Empty
or NotEmpty
checkers, or simply maybe ignored.
For this reason, and because I struggle in making it works with Empty
checker without conflicts in my current implementation, it seems simpler to add the Zero detection to existing Empty
checker. What do you think about it ?
@ccoVeille hi
I reread carefully the messages and examples above and looks like I was wrong
Empty
is concentrated on equals with len
.Zero
is concentrated on equals with zero value objects.And these are separate checkers, and zero
should cover not only zeros, but the full list:
If user uses assert.Equal(t, 0, len(warningMsg))
then we suggest assert.Empy(t, errors)
.
If uses uses assert.Equal(t, "", warningMsg)
then we suggest assert.Zero(t, warningMsg)
.
Also need to grep some statistics and understand the history of creation of Zero
.
To enable it by default or not? Really it useful or not?
Thanks for digging further into the checks.
There are definitely testify assert that covers the same kind of check.
I mean an assert.Empty(t, nilPointer)
, assert.Zero(t, nilPointer)
, is valid, but assert.Nil(t, nilPointer)
would be better.
Your tool is there to suggest using the right asserted.
So I think while each checker COULD handle them all, BUT your tool will have to provide opinionated suggestions, and document them.
For me, using Zero
on empty string would be strange, I would expect to see Empty
being suggested.
Considering a string is a list of character, you can often find people using len(emptyString) == 0
, while others are using emptyString == ""
So here is the same, Empty and Zero have lot in common, you will have to define what is the best for each type.
Let's continue the discussion
Also, as I previously said, I think empty and zero could be handled in the same checker, as they are very close.
I think the project would need something like this:
This list has to be completed, and discussed of course. I let out all the compare operators and co.
My approach is the opposite of "this checker does that". It's more a "what is expected from testifylint".
I'm not talking about how it's implemented, but what would be the good way to use testify. So, a guidance, it would be opinionated. It could be used as a reference for new checkers implementation.
I could write a design note in a markdown file you could review.
EDIT: 2024-05-28 Zero was completed with "zero time" use cases
You didn't reply to my suggestion and questions. It's OK. But I'm busy IRL for a while.
Could you please unassign me from the issue? Thanks
I think the project would need something like this:
Nil is suggested for
- Empty(nil)
- assert.Zero(nil)
NoError is suggested for
- Empty(nilErrorPointer)
- Zero(nilErrorPointer)
- Nil(nilErrorPointer)
Empty is suggested for
- Len(0, list)
- Zero(emptyString)
Zero is suggested for
- Empty(int) …
This list has to be completed, and discussed of course. I let out all the compare operators and co.
My approach is the opposite of "this checker does that". It's more a "what is expected from testifylint".
I'm not talking about how it's implemented, but what would be the good way to use testify. So, a guidance, it would be opinionated. It could be used as a reference for new checkers implementation.
I could write a design note in a markdown file you could review.
Hi @dolmen
I'm curious about your thoughts about this.
Thanks
@dolmen didn't reply and it's OK, he might be busy
Maybe, I should bring the discussion on https://gophers.slack.com/archives/C18A3680H
I also thought about opening a discussion on https://github.com/stretchr/testify/discussions
It would require me to work on the matrix I talked about https://github.com/Antonboom/testifylint/issues/75#issuecomment-2067622523 before opening the discussion of course
what do you think @Antonboom ?
time.Time has a https://pkg.go.dev/time#Time.IsZero function. How do you recommend checking a time.Time is zero ?
using assert.True(t, time.Time.IsZero()) or assert.Zero(t, time.Time) ?
time.Time has a https://pkg.go.dev/time#Time.IsZero function. How do you recommend checking a time.Time is zero ?
using assert.True(t, time.Time.IsZero()) or assert.Zero(t, time.Time) ?
Good remark, I will have to check what it does, but I would say assert.Zero(t, zeroTime)
would be more logic, and shorter
but I have to check if it works first
Just noticed @dolmen added a :+1: to https://github.com/Antonboom/testifylint/issues/75#issuecomment-2067589254
so I would say, we had a kind of approval :smile:
time.Time has a https://pkg.go.dev/time#Time.IsZero function. How do you recommend checking a time.Time is zero ?
using assert.True(t, time.Time.IsZero()) or assert.Zero(t, time.Time) ?
thanks @mmorel-35 for your feedback and your interest in the discussion I brought
time.Time has a https://pkg.go.dev/time#Time.IsZero function. How do you recommend checking a time.Time is zero ? using assert.True(t, time.Time.IsZero()) or assert.Zero(t, time.Time) ?
Good remark, I will have to check what it does, but I would say
assert.Zero(t, zeroTime)
would be more logic, and shorterbut I have to check if it works first
confirmed it works perfectly https://go.dev/play/p/LjKyrg0sn1H
So I would recommend using assert.Zero for checking zero time
thanks for your suggestion @mmorel-35
I edited https://github.com/Antonboom/testifylint/issues/75#issuecomment-2067622523
@ccoVeille , I found zero being suggested as a New linter for golangci-lint. It might be helpful to identify "Zero-able" types . It might worth having a look to start working on this feature?
The project is somehow abandoned, but it could be a good source of inspiration
I think the project would need something like this:
Nil is suggested for
- Empty(nil)
- Zero(nil)
NoError is suggested for
- Empty(nilErrorPointer)
- Zero(nilErrorPointer)
- Nil(nilErrorPointer)
Empty is suggested for
- Len(0, list)
- Zero(emptyString)
Zero is suggested for
- Empty(int)
- Empty(time.Time)
- Equal(time.Time, 0)
- True(t, time.Time.IsZero()) …
This list has to be completed, and discussed of course. I let out all the compare operators and co.
My approach is the opposite of "this checker does that". It's more a "what is expected from testifylint".
I'm not talking about how it's implemented, but what would be the good way to use testify. So, a guidance, it would be opinionated. It could be used as a reference for new checkers implementation.
I could write a design note in a markdown file you could review.
EDIT: 2024-05-28 Zero was completed with "zero time" use cases
@mmorel-35 do you think you could use the zero project to work on populating the document I started writing?
Do you agree with me that having such a table out of current checker consideration would be a good thing?
I'm busy with my new born kid 👨👩👧👧, I'm pretty active on GitHub but I'm mostly using my phone. Most of my time is spent I'm trying to make my baby girl sleep 😪
any help would be appreciated.
@ccoVeille , I found zero being suggested as a New linter for golangci-lint. It might be helpful to identify "Zero-able" types . It might worth having a look to start working on this feature?
@mmorel-35 by being suggested you meant here?
https://github.com/golangci/golangci-lint/issues/3491#issue-1548246245
@mmorel-35 I'm curious how zero
differs from what I found in testify and already talked about here
https://github.com/Antonboom/testifylint/issues/75#issuecomment-2041380821
I will check a bit further in the code of testify, but I think is what we should look for\n\nhttps://github.com/stretchr/testify/blob/master/assert/assertions_test.go#L23-L85\n\nI would say Zero checker should report to be used when using Equal, Equalf, Exactly and Exactlyf for the following type\n\ncomplex64(0), complex128(0),\nfloat32(0), float64(0),\nint(0), int8(0), int16(0), int32(0), int64(0),\nuint(0), uint8(0), uint16(0), uint32(0), uint64(0
do you think you could use the zero project to work on populating the document I started writing?
It seems to be close to what you started to identify. There probably be some differences of course
Do you agree with me that having such a table out of current checker consideration would be a good thing?
It's probably easier to have inside the project but probably isolated so it can be outside in a second time
I'm busy with my new born kid 👨👩👧👧, I'm pretty active on GitHub but I'm mostly using my phone. Most of my time is spent I'm trying to make my baby girl sleep 😪
any help would be appreciated.
Good luck with your child. I won't be able to help on the development part. I wanted to share what I found that could help the good improvements testifylint is providing .
@ccoVeille , I found zero being suggested as a New linter for golangci-lint. It might be helpful to identify "Zero-able" types . It might worth having a look to start working on this feature?
@mmorel-35 by being suggested you meant here?
Exactly here, yes
@ccoVeille
I don't see the benefits from wasting resources on support matrix above.
And we have a lot of more useful checkers (see issues list), that protect from real bugs and not just stylish thing. So, please turn your attention to them.
Thanks!
For records, I started working on the implementation of this feature request.
https://github.com/Antonboom/testifylint/blob/master/CONTRIBUTING.md#zero