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

Get citekey from future citationKey or extract from extra field #1814

Merged
merged 5 commits into from Jan 31, 2019

Conversation

@retorquere
Copy link
Contributor

commented Jan 29, 2019

No description provided.

@adam3smith
Copy link
Collaborator

left a comment

A couple of questions. Also, you need to bump timestamps, please.

citekey = buildCiteKey(item, citekeys);
}
var citekey = buildCiteKey(item, extraFields, citekeys);
var extraFields = item.extra ? parseExtraFields(item.extra) : null;

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

I always get confused in how JS handles variables, but for clarity we should define the variable before we use it, i.e. flip these two lines.

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

Oops. Change submitted. That was unintentional.

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

I've also bumped the dates.

@@ -1270,7 +1275,8 @@ function doExport() {
if (!type) type = "misc";

// create a unique citation key
var citekey = buildCiteKey(item, citekeys);
var extraFields = item.extra ? parseExtraFields(item.extra) : null;

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

here you have them in the right order, so I assume that's just a typo above

@@ -386,7 +386,12 @@ function creatorCheck(item, ctype) {
return false;
}

function buildCiteKey(item, citekeys) {
function buildCiteKey (item, extraFields, citekeys) {
const citationKey = extraFields.findIndex(field => field.field && field.value && field.field.toLowerCase() === 'citation key')

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

why is this citation key and not citationkey ? (I might be missing something obvious.)

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

As far as I can tell, https://github.com/zotero/translators/blob/master/BibTeX.js#L146 or anywhere after doesn't normalize the key, and according to #1810 (comment) the extra field would be Citation Key. Right? I did test and it seemed to work this way. I can add code to normalize the parsed field keys, but I don't know what else that would break.

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

we don't have a convention on this -- currently, all other fields we use Extra for, we just pick up the CSL variable name (for obvious reasons), but that doesn't exist here.

I don't have a strong view on how this should be named, but to keep it consistent with other fields in extra, should we name it what a CSL variable would be named, i.e. citation-key?
How do you currently suggest/require people enter keys in Extra for BBT?

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

I currently support bibtex: (for backwards compatibility) and Citation Key: (suggested by convention, because it's what BBT will add currently if you pin a key), but will follow any convention that Zotero proposes. If we settle for citation-key here, I'll just adjust BBT to pin keys that way. A re-pin would change bibtex: or Citation Key: to citation-key if present under that regime.

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

No need to multiply these then. Let's go with Citation Key: i.e. keep the code as is.

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

It's an easy change for me though, so if citation-key is a better philosophical fit, that's a 5-minute change for me with no further consequences.

This comment has been minimized.

Copy link
@dstillman

dstillman Jan 30, 2019

Member

Citation Key: is good.

const citationKey = extraFields.findIndex(field => field.field && field.value && field.field.toLowerCase() === 'citation key')
if (citationKey >= 0) return extraFields.splice(citationKey, 1)[0].value

if (item.citationKey) return item.citationKey

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

shouldn't this be above the complex parsing for simplicity?

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

You mean outside the buildCiteKey function? Or something else? Sorry, I don't quite follow, can you elaborate?

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

Oh wait, do you mean first item.citationKey and then check the extra field? My idea was that if the user took the effort to put it in the extra field, that would always take precedence.

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

yes, that's what I meant. I'm fine with leaving it as you have here. item.citationKey will never exists currently, though? Or has it already been generated here (sorry, didn't re-read code)? In that case, definitely go with the Extra solution as the first option as you have it.

This comment has been minimized.

Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

It's there in anticipation of citekeys arriving in Zotero as .citationKey (as discussed in #1810 (comment)) but in the interim, BBT will make its cite key available there.

@retorquere

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

This would have the side benefit that citation keys the user specified (whether generated by BBT or not) would show up in Overleaf.

retorquere added some commits Jan 30, 2019

CSL JSON id is useful for pandoc, TEI was already updated for .citati…
…onKey but didn't have the extra-field parser

@adam3smith adam3smith merged commit a340b9b into zotero:master Jan 31, 2019

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2019

Great; having the id in the CSL JSON for pandoc and having the citation key available via server API for Overleaf is going to be very useful. Thanks!

@zuphilip zuphilip referenced this pull request Jan 31, 2019

Closed

BibTeX not working! #1816

zuphilip added a commit that referenced this pull request Jan 31, 2019

adam3smith added a commit to adam3smith/translators that referenced this pull request Feb 1, 2019

Re-introduce changes from zotero#1814
But this time without breaking stuff...

adam3smith added a commit that referenced this pull request Feb 1, 2019

Re-introduce changes from #1814 (#1818)
But this time without breaking stuff...

GuyAglionby added a commit to GuyAglionby/translators that referenced this pull request Mar 30, 2019

Get citekey from future citationKey or extract from extra field (zote…
…ro#1814)

* BibLaTex, BibTeX, TEI, CSL JSON
* CSL JSON id is useful for pandoc; TEI was already updated for .citationKey but didn't have the extra-field parser

GuyAglionby added a commit to GuyAglionby/translators that referenced this pull request Mar 30, 2019

GuyAglionby added a commit to GuyAglionby/translators that referenced this pull request Mar 30, 2019

Re-introduce changes from zotero#1814 (zotero#1818)
But this time without breaking stuff...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.