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

[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

[gh73962]

@ndyakov hi，pls check this PR

[ndyakov]

@gh73962 feel free to add the additional check for time.Time and write two tests - one for time.Time and one for a custom struct that holds an integer (you can test with an empty int and with a value that is different than 0).

[gh73962]

@ndyakov pls review

[Copilot]

Pull Request Overview

This PR implements support for time.Time in the isEmptyValue function and updates related tests to verify correct behavior. Key changes include:

• Adding a new case for time.Time in isEmptyValue.
• Introducing a test struct (setOmitEmpty) that verifies omitting empty fields, including time.Time values.
• Refactoring numberStruct methods by renaming receiver variables and replacing a duplicate UnmarshalBinary with a new ScanRedis method.

Reviewed Changes

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

File	Description
commands_test.go	Added tests for new behavior with time.Time and updated numberStruct methods
commands.go	Extended isEmptyValue to properly handle time.Time structures

Comments suppressed due to low confidence (1)

commands_test.go:7678

• [nitpick] Consider revisiting the naming of ScanRedis to keep it consistent with MarshalBinary if their functionalities are analogous, or add a comment to clarify the intended difference in behavior between ScanRedis and UnmarshalBinary.

func (n *numberStruct) ScanRedis(str string) error {

[ndyakov]

Added one additional case, overall looks good.