FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

add precision option for datetime string dumping by f0ster · Pull Request #370 · Dynamoid/dynamoid · GitHub

add precision option for datetime string dumping - #370

Open
f0ster wants to merge 1 commit into
Dynamoid:masterfrom
f0ster:master
Open

add precision option for datetime string dumping#370
f0ster wants to merge 1 commit into
Dynamoid:masterfrom
f0ster:master

Conversation

f0ster commented Jul 27, 2019

Copy link
Copy Markdown

No description provided.

pboling left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks like a great enhancement!
Is there a reasonable way to test this new option?

Copy link
Copy Markdown
Member

Great, but the feature is missing specs. Please add specs for Dynamoid::Dumping module.

f0ster commented Jul 28, 2019

Copy link
Copy Markdown
Author

@andrykonchin @pboling added specs

# 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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

andrykonchin Jul 29, 2019
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
=> 620000

Here

            if dt_has_precision
              dt.utc

completely 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)

Comment thread lib/dynamoid/undumping.rb
@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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I believe it should be tested as well. It's a new feature - storing milliseconds when use string format for datetime.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree, I think this could be a clearer abstraction. FWIW, This was amended to make the current and additional spec coverage pass

end



Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

andrykonchin Jul 29, 2019
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

@f0ster Did you have a chance to look through my comments?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL