Fractional date bug 1361 #148
Conversation
| "dateInformation" => parse_attributes(r, content: "dateInformation") | ||
| }.compact | ||
| end | ||
| rescue |
There was a problem hiding this comment.
This is my first time seeing rescue nil?
Does this catch all exceptions? That might be too broad. If there are specific types of exceptions we need to guard against, it could be good to specify the type.
If this was more about providing a default value, nill should be returned if the if statement fails.
There was a problem hiding this comment.
Yes rescue nil is very broad, a quick google also suggests some articles it's quite slow.
What's the actual exception being thrown and from what bit? The time.iso8601?
There was a problem hiding this comment.
Yes, the rescue is for the Time.iso8601 method. It throws errors if the date format is wrong, whereas the Date.edtf method doesn't. I can narrow down the rescue to more specific exceptions though
| @@ -1653,4 +1653,12 @@ | |||
| ) | |||
| end | |||
|
|
|||
| it "Parses dates with fractional seconds" do | |||
There was a problem hiding this comment.
Can we add a new test as well, that also handles the ranges we talked about.
Simplest way might be to make the fixture actually just something like datacite-example-dates.xml
and then have both a fractional date in it and also a date with a range.
then add some expects for both.
Purpose
Fixed issue where dates with fractional times wouldn't be returned from API
closes: datacite/datacite#1361
Approach
Added Time.iso8601 to handle fractional times
Open Questions and Pre-Merge TODOs
Learning
Researched different ways to handle parsing dates with fractional times according to the schema. Found a class which looked promising, but ended up not being supported for the version ruby we're using. After a bit more research I settled on using the Time.iso8601 method.
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines: