-
Notifications
You must be signed in to change notification settings - Fork 747
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.
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
Updates to use preprint item type #3279
Conversation
Asyncify (and tested) all tranlators in the process. I didn't bother fixing translators that test for the existence of preprint item type because those will already produce correct results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, a couple comments but generally looks great!
Europe PMC.js
Outdated
@@ -361,9 +364,10 @@ var testCases = [ | |||
{ | |||
"type": "web", | |||
"url": "https://europepmc.org/article/PPR/PPR358366", | |||
"detectedItemType": "report", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like detectWeb
needs to be updated
Research Square.js
Outdated
@@ -328,4 +326,5 @@ var testCases = [ | |||
"items": "multiple" | |||
} | |||
] | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a Scaffold bug? Looks like we're getting an extra newline in every file. If so, sorry, will fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's Scaffold or lint -- certainly not something I'm doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the lines-around-comment
ES-lint condition that forces this on lint fix.
"date": "July 19, 2021", | ||
"date": "2024-03-21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some problem with the returned RIS - while this date is indeed what's contained there, it is an incorrect one (and the replaced value in the diff was actually correct).
I started thinking about pulling it from an alternative field like citation_publication_date
instead, but then noticed your reply in #3232, so maybe we can simply solve it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a problem with the RIS, yeah, we should fix it in a separate PR for that translator. Outside of the scope of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-ter happy to look at a PR for this though (make sure you work off an updated fork with this one committed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to close this off - the fix is proposed in #3292.
Asyncify (and tested) all tranlators in the process.
I didn't bother fixing translators that test for the existence of preprint item type because those will already produce correct results