Preserve millisecond values on times [Fixes #308] #390

Merged
merged 1 commit into from Mar 30, 2012

3 participants

@kpshek

I've taken a stab at ensuring millisecond values are preserved on times. Thanks for looking at this and let me how this looks.

@bkeepers

@kpshek thanks for the pull request.

@jnunemaker do you remember the original issue with this?

@jnunemaker
@kpshek

I went back through the commit history here on Github and found that originally, times were stored to millisecond precision (which is what BSON allows).

https://github.com/jnunemaker/mongomapper/blob/c22bbde4fa1cfbc310d79cb0e50203310ffb03d1/lib/mongo_mapper/support.rb#L183-184

However, note in this version the milliseconds were calculated using floating point arithmetic and rounding which will produce slightly different values than what was intended.

...which was then followed up in commit ad25155 where milliseconds were removed.

The commit messages indicates milliseconds were removed due to the aforementioned rounding issues and @jnunemaker not seeing a need to use millisecond precision for representing times in Mongo.

I have an application in which I'm dealing with date/times in which millisecond precision is crucial and thus have a need to store this level of precision in my time attributes. I'm guessing the commenter on commit ad25155 and the person who logged issue #308 are in similar situations. Additionally, Mongo itself supports millisecond precision.

@jnunemaker
@kpshek

Thanks for the information on a short term solution - I can easily use that in the interim.

Out of curiosity, do you have plans to incorporate the proposed pull request changes into master? Thanks again for being so responsive and looking into this with me.

@jnunemaker

I'm definitely not opposed to supporting it if mongo supports it. I guess my only question is backwards compatibility. I suppose we could just create a TimeWithoutMilliseconds type and recommend people change times to that when upgrading.

@bkeepers any thoughts?

@bkeepers

I'm good with it. I can't really think of any compatibility issues.

@jnunemaker
@kpshek

@jnunemaker Sounds good. Let me know how the tests against Harmony go or if you'd like another type.

@kpshek

@jnunemaker - have you had a chance to run your tests against Harmony yet?

@jnunemaker
@bkeepers bkeepers merged commit 029dd45 into mongomapper:master Mar 30, 2012
@bkeepers

Tests pass on harmony. Thanks for the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment