Implemented new TsTimeStamp type. Updated examples and tests.

[shaunsales]

Still to be implemented;

• Create a TsTimeBucket type with range checks
• Figure out if we want mixed auto and explicit timestamp support in TS.MADD
• Add better exception tests for all commands

[shaunsales]

Added the TsTimeBucket type with updated tests and examples.

[DvirDukhan]

@shaunsales
Great PR, Thanks
Sorry for the long time it took me to return a feedback
few comments:

1. the values:

        public static readonly TsTimeStamp MinValue = new TsTimeStamp(_minUnixMilliseconds);
        public static readonly TsTimeStamp MaxValue = new TsTimeStamp(_maxUnixMilliseconds);

Those members function with a dual purpose: value validation and replacing the - and + symbols, which are used in the module API.
I prefer not to have a dual purpose. As long as the module is not supporting negative values, the value validation is a welcome addition. However, instead of using the module native API, the current implementation sends actual values, which I want to avoid since module changes might change (break) the client behavior. I prefer that the properties will return the strings - and +
2. Having functions with default timestamp value (no timestamp parameter) - great addition
3. Please use and allow implicit casting whenever possible, so the user experience moving to this version will not change dramatically.

[shaunsales]

@shaunsales

Great PR, Thanks

Sorry for the long time it took me to return a feedback

few comments:

1. the values:

        public static readonly TsTimeStamp MinValue = new TsTimeStamp(_minUnixMilliseconds);

        public static readonly TsTimeStamp MaxValue = new TsTimeStamp(_maxUnixMilliseconds);

Those members function with a dual purpose: value validation and replacing the - and + symbols, which are used in the module API.

I prefer not to have a dual purpose. As long as the module is not supporting negative values, the value validation is a welcome addition. However, instead of using the module native API, the current implementation sends actual values, which I want to avoid since module changes might change (break) the client behavior. I prefer that the properties will return the strings - and +

I was thinking about this for a while when I was implementing the new timestamp and I think a good solution might be to hide the string conversion in an internal extension.

I want to avoid creating any additional backing fields in the struct as currently it can be represented as a single long.

The way I dealt with * was to simply assume we let Redis auto-timestamp when the user doesn't provide a timestamp and I think that works quite intuitively. I'd like to find a similar solution to +/-

2. Having functions with default timestamp value (no timestamp parameter) - great addition

Thanks.

3. Please use and allow implicit casting whenever possible, so the user experience moving to this version will not change dramatically.

I think we should be a little bit cautious with implicit casting, as it can cause confusion if you use out of range values. But that's more a usability thing. I'll have a look into it. Was there any specific casts you had in mind?

[DvirDukhan]

@shaunsales

1. I agree - the usage of -/+ should be internal and not exposed. The next version should not support string casting. Maybe consider having static instances in the TsTimeStamp class for TsTimeStamp.MinTS and TsTimeStamp.MinTS that you can send as in input a function as the timestamp parameter, compare the parameter agains them and decide if the send the -/+ string (if you have a match) or the parameter timestamp UnixMilliseconds value.
2. Regarding the casts - I saw in the tests that you explicitly calling the constructor of TsTimeStamp, I want to avoid this, that's all.

[shaunsales]

1. I agree - the usage of -/+ should be internal and not exposed. The next version should not support string casting. Maybe consider having static instances in the TsTimeStamp class for TsTimeStamp.MinTS and TsTimeStamp.MinTS that you can send as in input a function as the timestamp parameter, compare the parameter agains them and decide if the send the -/+ string (if you have a match) or the parameter timestamp UnixMilliseconds value.

Sure, I think that's a good compromise. I've been quite busy this week, but will try to get onto it this weekend.

2. Regarding the casts - I saw in the tests that you explicitly calling the constructor of TsTimeStamp, I want to avoid this, that's all.

Ok, no problem - I was mostly wanting to point out that it could be confusing to users if they are using implicit casts and get ArgumentOutOfRangeException - my thinking was I wanted to have usability similar to the DateTime struct, so the learning curve is easier. When you explicitly construct, we can warn you about out-of-range values.

[DvirDukhan]

@shaunsales eventually an exception will be thrown if the user will try to use an invalid value when calling a function requiring TsTimeStamp instance (due to casting).
maybe add a unit test to the class for invalid usage from implicit casting?

[shaunsales]

@DvirDukhan Yep, let's do that. I'll let you know when the changes have been made. Thanks.

[DvirDukhan]

@shaunsales is your last commit handles the review comments? I can re-review?

[shaunsales]

@shaunsales is your last commit handles the review comments? I can re-review?

Sure. I believe all the outstanding requests have been implemented.

[DvirDukhan]

@shaunsales LGTM

[DvirDukhan]

@shaunsales
Anything else needs to be done?
If not, can you please resolve the conflicts so I'll approve?