search

mfenniak / rethinkdb-net

A C# / .NET client driver for RethinkDB.
Other
247 stars 37 forks source link

Add support for int, int?, long, and long? #84

Closed karlgrz closed 11 years ago

karlgrz commented 11 years ago

Since rethinkdb translates down to float anyway, I just used double / double? for the actual value.

mfenniak commented 11 years ago

Hi Karl,

Thanks so much for the pull request. I have a few changes I'd like you to make before I merge this; I hope you don't mind making a few adjustments.

I didn't originally support these types because they're not natively stored in RethinkDB in the correct type format. I'd like to see something done to protect against unexpected behaviour here because RethinkDB doesn't store ints or longs. There are two types of dangerous behaviour here:

I think that rethinkdb-net should detect any cases where the native int/long type won't be correctly stored or retrieved from an R_NUM type, and we should either throw exceptions or log warnings. I'd favour throwing exceptions, at least initially. There should be appropriate unit tests for these cases.

Also, I'm a bit picky in keeping consistent coding style; these are obviously less important changes, but I'd appreciate if you wouldn't mind adjusting them:

Linking this to issue #54; this PR adds some of the primitives that issue #54 documents as missing.

karlgrz commented 11 years ago

Hey Mathieu,

I don't mind making the adjustments at all. No problem.

I was reading up on the way rethinkdb handles ints and longs a bit as well and completely agree with your points. I will clean this all up this evening.

As for the formatting, I completely understand the pickiness! That's great, in my opinion. I actually did the coding and testing on Windows and pushed from ubuntu (I don't have mono 3.0 yet on my ubuntu vm). I tried to make sure I got all the nonsense out of there, but I will post more attention to that in the future.

Karl On Apr 12, 2013 6:18 PM, "Mathieu Fenniak" notifications@github.com wrote:

Hi Karl,

Thanks so much for the pull request. I have a few changes I'd like you to make before I merge this; I hope you don't mind making a few adjustments.

I didn't originally support these types because they're not natively stored in RethinkDB in the correct type format. I'd like to see something done to protect against unexpected behaviour here because RethinkDB doesn't store ints or longs. There are two types of dangerous behaviour here:

  • When receiving a R_NUM from RethinkDB, values might be not be representable as an int/long, either because they're fractional (eg. 0.5) , or because they're too large in magnitude (eg.1e300).
  • When sending a value to RethinkDB, the value might not be accurately representable as a double, either. For example, if a value reaches around 2^53, adding 1 to a long will change it's value, but adding 1 to a double will not.

I think that rethinkdb-net should detect any cases where the native int/long type won't be correctly stored or retrieved from an R_NUM type, and we should either throw exceptions or log warnings. I'd favour throwing exceptions, at least initially. There should be appropriate unit tests for these cases.

Also, I'm a bit picky in keeping consistent coding style; these are obviously less important changes, but I'd appreciate if you wouldn't mind adjusting them:

  • TestObject3.cs needs to be updated to have 4-character space indentation to be consistent with other files.
  • In PrimitiveDatumConverterFactory, "typeof" shouldn't have a space after it, "StringDatumConverter" has been de-indented, and object initializers have had spaces removed inside the braces.

Linking this to issue #54https://github.com/mfenniak/rethinkdb-net/issues/54; this PR adds some of the primitives that issue #54https://github.com/mfenniak/rethinkdb-net/issues/54documents as missing.

— Reply to this email directly or view it on GitHubhttps://github.com/mfenniak/rethinkdb-net/pull/84#issuecomment-16322466 .

karlgrz commented 11 years ago

Take a look, this is a bit closer.

mfenniak commented 11 years ago

Hi Karl,

Looks great. One note, there are .NET values for the max & min value of a long; System.Int64.MinValue, and System.Int64.MaxValue. Could you please adjust your branch to use those values instead and remove the long datum converter constants?

mfenniak commented 11 years ago

Excellent, thanks for merging this @karlgrz. I missed your last update with the constant changes, otherwise I would've merged it then. :+1: