Skip to content

Fractional date bug 1361 #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bklaing2 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fractional date bug 1361 #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cb2ae3
Add rspec test for fractional date
richardhallett Oct 4, 2022
670523d
Fixed the fractional date issue
bklaing2 Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions lib/bolognese/readers/datacite_reader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,15 @@ def read_datacite(string: nil, **options)

dates = Array.wrap(meta.dig("dates", "date")).map do |r|
if r.is_a?(Hash) && date = sanitize(r["__content__"]).presence
if Date.edtf(date).present? || Bolognese::Utils::UNKNOWN_INFORMATION.key?(date)
{ "date" => date,
"dateType" => parse_attributes(r, content: "dateType"),
"dateInformation" => parse_attributes(r, content: "dateInformation")
}.compact
begin
if Date.edtf(date).present? || Bolognese::Utils::UNKNOWN_INFORMATION.key?(date) || Time.iso8601(date).present?
{ "date" => date,
"dateType" => parse_attributes(r, content: "dateType"),
"dateInformation" => parse_attributes(r, content: "dateInformation")
}.compact
end
rescue
Copy link
Contributor

@jrhoads jrhoads Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@richardhallett richardhallett Oct 14, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@bklaing2 bklaing2 Oct 18, 2022

Choose a reason for hiding this comment

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

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

nil
end
end
end.compact
Expand Down
23 changes: 23 additions & 0 deletions spec/fixtures/datacite-example-fractional-date.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<resource xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="http://datacite.org/schema/kernel-4" xsi:schemaLocation="http://datacite.org/schema/kernel-4 http://schema.datacite.org/meta/kernel-4.4/metadata.xsd">
<identifier identifierType="DOI">10.5072/example-fractional-date</identifier>
<creators>
<creator>
<creatorName>Claire L O?Brien</creatorName>
</creator>
</creators>
<titles>
<title>Impact of Colonoscopy Bowel Preparation on Intestinal Microbiota</title>
</titles>
<dates>
<date dateType="Updated">2020-11-06T21:37:33.12Z</date>
</dates>
<publisher>The Australian National University Data Commons</publisher>
<publicationYear>2013</publicationYear>
<rightsList>
<rights xml:lang="en-US" schemeURI="https://spdx.org/licenses/" rightsIdentifierScheme="SPDX" rightsIdentifier="CC0 1.0" rightsURI="http://creativecommons.org/publicdomain/zero/1.0/"/>
</rightsList>
<language>en</language>
<resourceType resourceTypeGeneral="Dissertation"></resourceType>
</resource>
8 changes: 8 additions & 0 deletions spec/readers/datacite_reader_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1653,4 +1653,12 @@
)
end

it "Parses dates with fractional seconds" do
Copy link
Contributor

@richardhallett richardhallett Oct 14, 2022

Choose a reason for hiding this comment

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

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.

input = fixture_path + "datacite-example-fractional-date.xml"
subject = Bolognese::Metadata.new(input: input)
expect(subject.valid?).to be true
expect(subject.dates).to eq([{"date"=>"2020-11-06T21:37:33.12Z", "dateType"=>"Updated"}, {"date"=>"2013", "dateType"=>"Issued"}])
end


end