Skip to content
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

Improvements for Dataset & DOI handling #1338

Merged
merged 3 commits into from Jun 22, 2017

Conversation

@adam3smith
Copy link
Collaborator

commented Jun 19, 2017

Note for SAE: this appears currently broken due to invalid HTML
@zuphilip if you have time to review, that'd be great

Improvements for Dataset & DOI handling
Note for SAE: this appears currently broken due to invalid HTML
item.extra += "\nDOI: " + DOI;
}
} else {
item.extra = "DOI: " + DOI;

This comment has been minimized.

Copy link
@zuphilip

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.

Copy link
@adam3smith

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.

Copy link
@zuphilip

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);
	});
Figshare.js Outdated
item.extra = "DOI:" + DOI;
else {
if (item.extra) {
if (item.extra.search(/^DOI:/) == -1) {

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jun 19, 2017

Collaborator

Maybe we could here even do

if (item.extra.search(/(^|\n)DOI:/) == -1) {

This comment has been minimized.

Copy link
@adam3smith

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.

Copy link
@zuphilip

zuphilip Jun 19, 2017

Collaborator

Hm... this could also be...

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jun 20, 2017

Author Collaborator

No, you were right; changing

Figshare.js Outdated
}
} else {
item.extra = "DOI: " + DOI;
}

This comment has been minimized.

Copy link
@zuphilip

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.

Copy link
@adam3smith

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.

Copy link
@zuphilip

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.

Copy link
@adam3smith

adam3smith Jun 19, 2017

Author Collaborator

because the RDF translator handles all other dublin core code.

This comment has been minimized.

Copy link
@zuphilip

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.

Copy link
@adam3smith

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.

Copy link
@zuphilip

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.

Copy link
@adam3smith

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.

Copy link
@adam3smith

adam3smith Jun 20, 2017

Author Collaborator

here I was right: testing shows that "type: dataset" works, itemType: dataset doesn't. Changed in the documentation

Zenodo.js Outdated

//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.

Copy link
@zuphilip

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.

Copy link
@adam3smith

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.

Copy link
@zuphilip

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.)

Zenodo.js Outdated
var zoteroType = {
"figure": "artwork",
"article": "report"
}

This comment has been minimized.

Copy link
@zuphilip

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.

Copy link
@adam3smith

adam3smith Jun 20, 2017

Author Collaborator

done

Zenodo.js Outdated
var zoteroType = {
"figure": "artwork",
"article": "report"
}

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jun 19, 2017

Collaborator

What about data set?

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jun 19, 2017

Author Collaborator

see above -- I'm leaving that as document on purpose (though feel free to disagree

Zenodo.js Outdated
if (abstract) item.abstractNote = abstract;
if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];
}

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jun 19, 2017

Collaborator

Maybe add a comment here, what these lines are doing and why.

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jun 20, 2017

Author Collaborator

done

// 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.

Copy link
@zuphilip

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.

Copy link
@adam3smith

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.

@adam3smith

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2017

Thanks, updated with your comments.

Zenodo.js Outdated
trans.itemType = type;
trans.doWeb(doc, url);

if (!item.DOI) {

This comment has been minimized.

Copy link
@zuphilip

zuphilip Jun 20, 2017

Collaborator
if (!item.DOI && doi) {
@zuphilip

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2017

This looks good. Just some suggestions/comments/replies.

@adam3smith adam3smith merged commit be399e3 into zotero:master Jun 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zuphilip added a commit to zuphilip/translators that referenced this pull request Mar 28, 2018

Improvements for Dataset & DOI handling (zotero#1338)
Note for SAE: this appears currently broken due to invalid HTML

zuphilip added a commit to zuphilip/translators that referenced this pull request Mar 28, 2018

Improvements for Dataset & DOI handling (zotero#1338)
Note for SAE: this appears currently broken due to invalid HTML
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.