| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
This looks like a great enhancement!
Is there a reasonable way to test this new option?
Sorry, something went wrong.
|
Great, but the feature is missing specs. Please add specs for Dynamoid::Dumping module. |
Sorry, something went wrong.
| # parse first for stricter constraint failure before attempting to parse iso for precision support | ||
| # e.g. DateTime.iso8601("2018-12") is valid | ||
| 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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)
Sorry, something went wrong.
| @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.
Sorry, something went wrong.
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
Sorry, something went wrong.
| end | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Excessive new lines detected
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
|
@f0ster Did you have a chance to look through my comments? |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
No description provided.