-
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
BibTeX/RIS: Add DOIs to non-supported item types #1233
Conversation
BibTeX: also add additional test, udpated tests, and add better handling of additional identifiers from notes (which were in my local bibtex either from an old version by Emiliano or from an attempt by me; either way they look good).
Also, some minor touch-ups like cleaning out tabs and spaces from empty lines, adding semicolons, removing deprecated code.
update tests (manually)
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.
Some comments are below. This looks good for me in general.
@@ -331,13 +335,10 @@ function doSearch(item) { | |||
} else { | |||
var co = Zotero.Utilities.createContextObject(item); | |||
} | |||
|
|||
Zotero.Utilities.HTTP.doGet("http://www.crossref.org/openurl/?pid=zter:zter321&"+co+"&noredirect=true&format=unixref", function(responseText) { |
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.
This function call looks also a bit strange. Normally we just have ZU.doGet
and I would not be sure that the ZU.HTTP
functions are (always) available in translator code.
item.DOI = ZU.xpathText(refXML, 'c:doi_data/c:doi', ns); | ||
//add DOI to extra for unsupprted items | ||
if (item.DOI && item.itemType != "journalArticle" && item.itemType != "conferencePaper") { | ||
item.extra = "DOI: " + item.DOI; |
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.
How about switching here also to the more future-proof version instead of the one-liner as you have in the others already?
if (item.extra) {
item.extra += '\nDOI: ' + value;
} else {
item.extra = 'DOI: ' + value;
}
@@ -1451,6 +1452,19 @@ function applyValue(item, zField, value, rawLine) { | |||
item.extra = value; | |||
} | |||
break; | |||
case 'DOI': | |||
if (!ZU.fieldIsValidForType("DOI", item.itemType) && ZU.cleanDOI(value)) { | |||
value = ZU.cleanDOI(value); |
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.
Nits: If you switch these two lines, then you only have to calculate cleanDOI
once.
else { | ||
item[zField] = value; | ||
} | ||
break; |
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.
Alternatively, you can delete the else
above and just continue without a break
, which means that for DOI
all default
things will be performed and (depending on the itemType
) also some more, i.e. delete these lines
else {
item[zField] = value;
}
break;
@@ -262,7 +263,14 @@ function setKeywordDelimRe( val, flags ) { | |||
function processField(item, field, value, rawValue) { | |||
if(Zotero.Utilities.trim(value) == '') return null; | |||
if(fieldMap[field]) { | |||
item[fieldMap[field]] = value; | |||
//map DOIs + Label to Extra for unsupported item types | |||
if (field == "doi" && item.itemType != "journalArticle" && item.itemType != "conferencePaper") { |
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.
I like the (more general) function ZU.fieldIsValidForType("DOI", item.itemType)
which you used in another translator. How about using it here as well?
item.DOI = ZU.xpathText(refXML, 'c:doi_data/c:doi', ns); | ||
//add DOI to extra for unsupprted items | ||
if (item.DOI && item.itemType != "journalArticle" && item.itemType != "conferencePaper") { |
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.
I like the (more general) function ZU.fieldIsValidForType("DOI", item.itemType)
which you used in another translator. How about using it here as well?
@@ -262,7 +263,14 @@ function setKeywordDelimRe( val, flags ) { | |||
function processField(item, field, value, rawValue) { | |||
if(Zotero.Utilities.trim(value) == '') return null; | |||
if(fieldMap[field]) { | |||
item[fieldMap[field]] = value; | |||
//map DOIs + Label to Extra for unsupported item types |
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.
We could also let the first line as it was and just do some extra work for the doi, i.e. doing the else
case below always.
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.
I'll leave this here -- I'd like to leave the DOI in the generic field mapping (l. 84) and the only way to get to else
would be to remove it there.
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.
Not sure we understand each other here. My suggestion would look like this:
if(fieldMap[field]) {
//default behaviour
item[fieldMap[field]] = value;
//additionally map DOIs + Label to Extra for unsupported item types
if (!ZU.fieldIsValidForType("DOI", item.itemType)) {
var label = extraIdentifiers[field];
item._extraFields.push({field: label, value: value.trim()});
}
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.
Ah I see. I figured it'd be better if the item isn't mapped to item.DOI when it's written to Extra, so that we don't get DOI twice when using bibtex from a web translator that also adds DOIs (I know a couple do). What's the advantage of doing this your way around?
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.
Unsupported fields will not be saved in Zotero such that there will never be two fields containing the DOI information. No?
Anyway, there is no big deal here. I just thought it might be easier to read (and maybe already work correctly in some future version of Zotero item types).
I will review your changes a little later, but I am sure everything is good.
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.
but they're available in the translator object before saving, so something like this in a web translator:
https://github.com/zotero/translators/blob/master/Springer%20Link.js#L158
would cause DOI: 10.1234/123456 to appear twice in the Extra field.
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.
ok
have another look, please @zuphilip
else { | ||
item[zField] = value; | ||
} | ||
break; |
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.
This change I caused with my previous comment, might produce now that the doi data will be saved in item.extra
as well as in item.DOI
in the translator obejct as you outlined in #1233 (comment) . Thus, we might switch back here again to the else case and the break to prevent that.
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.
I think I have an elegant solution now moving the break
into the if condition
@@ -2069,7 +2065,7 @@ var testCases = [ | |||
"abstractNote": "Abstract", | |||
"archive": "Name of Database", | |||
"archiveLocation": "Accession Number", | |||
"extra": "Publication Number", | |||
"extra": "DOI: 10.1234/123456\nPublication Number", |
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.
Why did this value vanish now?
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.
I don't understand -- it's still there after the line break.
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.
Sorry, my fault. Somehow I switched the meaning of green and red here in my mind. The doi is here, which is good.
@@ -293,7 +293,12 @@ function processCrossRef(xmlOutput) { | |||
item.DOI = ZU.xpathText(refXML, 'c:doi_data/c:doi', ns); | |||
//add DOI to extra for unsupprted items | |||
if (item.DOI && item.itemType != "journalArticle" && item.itemType != "conferencePaper") { | |||
item.extra = "DOI: " + item.DOI; |
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.
How about using !ZU.fieldIsValidForType("DOI", item.itemType)
here as well?
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.
somehow forgot this; done.
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 good!
great, thanks |
Thank you @adam3smith for the work! This looks really great. |
BibTeX: also add additional test, udpated tests, and add better handling of additional identifiers from notes (which were in my local bibtex either from an old version by Emiliano or from an attempt by me; either way they look good). * CrossRef: Also, some minor touch-ups like cleaning out tabs and spaces from empty lines, adding semicolons, removing deprecated code.
BibTeX: also add additional test, udpated tests, and add better handling of additional identifiers from notes (which were in my local bibtex either from an old version by Emiliano or from an attempt by me; either way they look good). * CrossRef: Also, some minor touch-ups like cleaning out tabs and spaces from empty lines, adding semicolons, removing deprecated code.
BibTeX: also add additional test, udpated tests, and add better
handling of additional identifiers from notes (which were in my local
bibtex either from an old version by Emiliano or from an attempt by me;
either way they look good).