add precision option for datetime string dumping #370
Conversation
|
Great, but the feature is missing specs. Please add specs for |
|
@andrykonchin @pboling added specs |
| DateTime.parse(value) | ||
| DateTime.iso8601(value) |
There was a problem hiding this comment.
I don't think we need these changes in type casting. Anything that is like date/time could be casted to DateTime. Type casting isn't connected with writing/reading attributes to/from DynamoDB.
There was a problem hiding this comment.
@andrykonchin Per the comment, the existing expected behaviour is to break for poorly parsed strings. https://github.com/Dynamoid/dynamoid/blob/master/spec/dynamoid/type_casting_spec.rb#L143
I wrote this work around to support precision, and also keep the same erroneous expectations. Parse is expected to throw an exception, where as constructing with iso8601 has less strict input expectations.
There was a problem hiding this comment.
I see. Well.
DateTime.parse(value)
DateTime.iso8601(value)Here. DateTime.parse supports precision, doesn't it?
irb(main):003:0> s = DateTime.now.iso8601(2)
=> "2019-07-29T22:06:24.62+03:00"
irb(main):004:0> DateTime.parse(s).usec
=> 620000Here
if dt_has_precision
dt.utccompletely ignores ApplicationTimeZone.utc_offset what's wrong. If passed string doesn't have timezone - ApplicationTimeZone.utc_offset should be used as default timezone.
I thinks precision support could be added directly in this line:
DateTime.new(dt.year, dt.mon, dt.mday, dt.hour, dt.min, dt.sec, offset)seconds argument could have fraction so this line could be changed in this way:
DateTime.new(dt.year, dt.mon, dt.mday, dt.hour, dt.min, dt.sec + dt.sec_fraction, offset)Look at the example:
irb(main):015:0> s = DateTime.now.iso8601(4)
=> "2019-07-29T22:24:20.8163+03:00"
irb(main):016:0> DateTime.parse(s).sec
=> 20
irb(main):018:0> DateTime.parse(s).sec_fraction
=> (8163/10000)| @@ -202,7 +202,7 @@ def process(value) | |||
| else | |||
| @options[:store_as_string] | |||
| end | |||
| value = DateTime.iso8601(value).to_time.to_i if use_string_format | |||
| value = DateTime.iso8601(value).to_time if use_string_format | |||
There was a problem hiding this comment.
I believe it should be tested as well. It's a new feature - storing milliseconds when use string format for datetime.
There was a problem hiding this comment.
I agree, I think this could be a clearer abstraction. FWIW, This was amended to make the current and additional spec coverage pass
| @@ -239,6 +239,8 @@ | |||
| Dynamoid.config.store_datetime_as_string = store_datetime_as_string | |||
| end | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Excessive new lines detected
|
|
||
| obj = klass.create(signed_up_on: signed_up_on) | ||
|
|
||
| expect(reload(obj).signed_up_on.to_f).to eql(DateTime.iso8601(signed_up_on).to_f) |
There was a problem hiding this comment.
I think i't completely OK to use to_f here but it's a bad practice to check equality of floating point numbers. Comparing milliseconds or string representation would be more correct.
The precision is 6 in the test so it's OK, but with precision 9 (with nanoseconds) this test could become flaky because MRI isn't accurate in preserving nanoseconds for Time/DateTime.
|
@f0ster Did you have a chance to look through my comments? |
No description provided.