feat: func isEmptyValue support time.Time

[gh73962]

This PR is based on the solution discussed in the #3208 issue.

[ndyakov]

@gh73962 Thank you for the contribution. The test that you have added would have passed with or without the addition you did. Can we check what will be read so we verify the data. Maybe a raw read would work, since if we unmarshal the response to the object it would be the same as before, won't it?

[gh73962]

@gh73962 Thank you for the contribution. The test that you have added would have passed with or without the addition you did. Can we check what will be read so we verify the data. Maybe a raw read would work, since if we unmarshal the response to the object it would be the same as before, won't it?

Of course, I don't have the capability to perform this kind of test on my side. Changes like this should have comprehensive tests to ensure code quality.

[ndyakov]

@gh73962 would you like me to help you add those tests? The CI will run a redis server, you should be able to execute code agains it in the tests (you can check the existing tests for Set and Get). Let me know if I can help you somehow.

[gh73962]

@gh73962 would you like me to help you add those tests? The CI will run a redis server, you should be able to execute code agains it in the tests (you can check the existing tests for Set and Get). Let me know if I can help you somehow.

Of course

[ndyakov]

@gh73962 any concrete questions on how to write tests? will you be able to do this on your own?

[gh73962]

In my first commit of the commands_test.go file, the added test covers a scenario that our team frequently encounters.I need you guide, What additional test cases should I include?

[Copilot]

PR Overview

This PR adds support in the isEmptyValue function to correctly handle time.Time values by leveraging Go’s reflect.IsZero.

• Updated commands.go to use v.IsZero() for reflect.Struct to check for zero values.
• Extended commands_test.go with a new test struct (set2) including a time.Time field to validate the new functionality.

Reviewed Changes

File	Description
commands.go	Added reflect.Struct case using v.IsZero() for isEmptyValue.
commands_test.go	Added a test struct with a time.Time field to exercise the new functionality.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

commands_test.go:2581

• Consider adding a test case where the time.Time value is set to a non-zero value to confirm that non-empty time values are handled correctly.

Set5 time.Time     `redis:"set5,omitempty"`

commands.go:156

• Review whether applying v.IsZero() to all structs might have unintended consequences for non-time.Time types. If this branch is intended only for time.Time support, consider verifying or isolating behavior for other struct types.

case reflect.Struct:

[ndyakov]

@gh73962 lets add tests for the following scenarios:

• Add a test with custom struct in the hash that wants to hold a zero value (for example type RequestsSetting struct { Limit int } and the limit set to zero. Check if the struct is parsed correctly when the data is requested from redis.
• Add a test which validates that the data that was received from redis is what was written to redis. For example add a test where you set the time.Time value and validate that the same is parsed later when reading the data.

[gh73962]

@gh73962 lets add tests for the following scenarios:

* Add a test with custom struct in the hash that wants to hold a zero value (for example `type RequestsSetting struct { Limit int }` and the limit set to zero. Check if the struct is parsed correctly when the data is requested from redis.

* Add a test which validates that the data that was received from redis is what was written to redis. For example add a test where you set the `time.Time` value and validate that the same is parsed later when reading the data.

add to commands_test.go It("should HSet", func() ?

[ndyakov]

@gh73962 yes.

[ndyakov]

@gh73962 can we try with a child struct? Would you like me to write the tests or you would do it?

[ndyakov]

I don't find the testing sufficient. Please check the discussion so far.

[gh73962]

@ndyakov I was sick a few days ago, and I'll start improving these tasks tomorrow.

[gh73962]

@gh73962 can we try with a child struct? Would you like me to write the tests or you would do it?

I can test it out and check the impact on the child struct. Should I write the tests, or will you?

[gh73962]

@ndyakov pls review and hset.Scan not support struct

[ndyakov]

If we consider struct as unsupported in

go-redis/internal/hscan/hscan.go

Line 21 in 0df0f3c

decoders = []decoderFunc{

, should we filter better for time.Time here ? @gh73962

[gh73962]

code will be

    case reflect.Struct:
        if v.Type() == reflect.TypeOf(time.Time{}) {
            return v.IsZero()
        }
        // Only supports the struct time.Time, 
        // subsequent iterations will follow the func Scan support decoder.

What do u think about this change?

[gh73962]

@ndyakov pls review and hset.Scan not support struct

Additional note: hGetAll.Scan does not support parsing into nested structs, not that Scan doesn't support structs in general. The current issue is that hset supports implementing the BinaryMarshaler interface, while Scan fails to support nested BinaryUnmarshaler, creating an asymmetry in functionality. I believe redisv9's hset and scan are very developer-friendly, which is why my team migrated from v6 to v9. I hope go-redis can align the capabilities of hset and scan in the future. Thank you very much.
@ndyakov