Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upImprovements for Dataset & DOI handling #1338
Conversation
zuphilip
reviewed
Jun 19, 2017
item.extra += "\nDOI: " + DOI; | ||
} | ||
} else { | ||
item.extra = "DOI: " + DOI; |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
This should not be needed because it should be already covered by the changes in EM translator.
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 20, 2017
Author
Collaborator
it's needed because this is recognized by EM as a journal article so it doesn't move the DOI. We change that above.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 20, 2017
Collaborator
We could also change the itemType
in the translatorObject cf. https://github.com/zotero/translators/blob/master/Embedded%20Metadata.js#L815 and then (I assume) it is not necessary anymore. I.e.
translator.getTranslatorObject(function(trans) {
trans.itemType = type;
trans.doWeb(doc, url);
});
item.extra = "DOI:" + DOI; | ||
else { | ||
if (item.extra) { | ||
if (item.extra.search(/^DOI:/) == -1) { |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
Maybe we could here even do
if (item.extra.search(/(^|\n)DOI:/) == -1) {
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 19, 2017
Author
Collaborator
I haven't tested, but shouldn't ^DOI find the string at the beginning of any new line?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
} | ||
} else { | ||
item.extra = "DOI: " + DOI; | ||
} |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
Wouldn't it be better to do this in the called translator rather then the special one here?
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 20, 2017
Author
Collaborator
turns out the whole passage isn't necessary -- the DOI is in the RIS and the RIS translator does handle this now.
else newItem.extra = "type: dataset"; | ||
} | ||
|
||
|
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
Why did you do this change in the RDF translator? Is this the best place for this?
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 19, 2017
Author
Collaborator
because the RDF translator handles all other dublin core code.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
I haven't thought this through, but RDF is usually called by EM. Is there also another (common) use case for RDF translator?
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 20, 2017
Author
Collaborator
Common no. General -- other RDF imports, including Zotero RDF, DC RDF, and Bibliontology RDF. Arguably those should all recognize the (valid) DC type dataset.
if (newItem.extra) { | ||
newItem.extra += "\ntype: dataset"; | ||
} | ||
else newItem.extra = "type: dataset"; |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
Shouldn't this be newItem.extra = "itemType: dataset"
as documented here https://www.zotero.org/support/dev/translators/datasets ? Otherwise we should update the documentation.
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 19, 2017
Author
Collaborator
again, I need to test this, but I'm pretty sure type is correct here, as this is used by citeproc and hence relies on CSL json (which uses type, not itemType).
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 20, 2017
Author
Collaborator
here I was right: testing shows that "type: dataset" works, itemType: dataset
doesn't. Changed in the documentation
|
||
//something is odd with zenodo's author parsing; fix it | ||
for (var i = 0; i< item.creators.length; i++) { | ||
if (!item.creators[i].firstName && item.creators[i].lastName.indexOf(",")!=-1) { |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
The authors in CSL JSON look fine for me and also importing the CSL JSON works for me correctly. Can you give an example where this fix is needed?
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 19, 2017
Author
Collaborator
https://www.zenodo.org/record/569304/export/csl#.WUgtNsaQy70
but it's also in one of the tests. I don't understand the problem -- have double-checked data entry for this one and we did that exactly as recommended.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
Okay, I haven't seen this example. But I cannot find it in any test case...
(BTW I read the comment above also, that there is always something wrong with zenodo's author metadata, which is not the case.)
var zoteroType = { | ||
"figure": "artwork", | ||
"article": "report" | ||
} |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
I would suggest to move this to the place below where it is actually used. Otherwise I will have forgotten what this is when reading the code below...
This comment has been minimized.
This comment has been minimized.
var zoteroType = { | ||
"figure": "artwork", | ||
"article": "report" | ||
} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 19, 2017
Author
Collaborator
see above -- I'm leaving that as document on purpose (though feel free to disagree
if (abstract) item.abstractNote = abstract; | ||
if (item.itemType == "document" && zoteroType[type]) { | ||
item.itemType = zoteroType[type]; | ||
} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
// Add DOI to non-supported item types | ||
if (newItem.DOI && !ZU.fieldIsValidForType("DOI", newItem.itemType)) { | ||
if (newItem.extra){ | ||
newItem.extra += "\nDOI: " + newItem.DOI; |
This comment has been minimized.
This comment has been minimized.
zuphilip
Jun 19, 2017
Collaborator
Maybe also check that we only do this once, i.e.
if (item.extra.search(/(^|\n)DOI:/) == -1) {
as below.
This comment has been minimized.
This comment has been minimized.
adam3smith
Jun 20, 2017
Author
Collaborator
Since there is no way for the Extra field to contain anything other than note (given the CSL import from Zotero) I just empty the field above and can do without all this code.
This comment has been minimized.
This comment has been minimized.
Thanks, updated with your comments. |
zuphilip
reviewed
Jun 20, 2017
trans.itemType = type; | ||
trans.doWeb(doc, url); | ||
|
||
if (!item.DOI) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This looks good. Just some suggestions/comments/replies. |
adam3smith commentedJun 19, 2017
Note for SAE: this appears currently broken due to invalid HTML
@zuphilip if you have time to review, that'd be great