Require time lib otherwise Time.iso8601 is undefined by default#38
Merged
Conversation
In the `Util.deep_parse_iso8601_values` we reference `Time.iso8601` but to use that in Ruby you need to require the time library otherwise the method is undefined. The implementation has a `rescue` but that was unintended since its only meant to capture cases where a string can’t be converted to a time object. We weren’t seeing any errors here because it also caught the undefined error and let the time be a string value.
Contributor
|
Nice catch @javierjulio! Thanks. Version |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While I was working on the changes in #37 I noticed that the timestamps in responses remained as strings but I had already seen the middleware and knew it was supposed to be converted. The issue is that while I was in a
bundle consolethe library is loaded as you have it defined but when using this in say a Rails app you wouldn't notice the issue as the time lib is already required by Rails. This just makes it a clear dependency here and now you'll see times being converted. Before the change it was:"created"=>"2018-01-03T23:42:08.963Z"}becauseTime.iso8601is an undefined but since it also had arescuestatement it was swallowing that error. Now including the changes in this PR it came out to:"created"=>2018-01-03 23:42:08 UTC}so you can see its being converted.