Skip to content

/translators

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

Reviewers

@adam3smith adam3smith

@dstillman dstillman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@retorquere @adam3smith @dstillman
@retorquere
Copy link
Contributor

retorquere commented Jan 29, 2019

No description provided.

@retorquere
extract citekey
Loading status checks…
81cf690
@adam3smith

adam3smith requested changes Jan 30, 2019
View changes

Copy link
Collaborator

adam3smith left a comment

A couple of questions. Also, you need to bump timestamps, please.
BibLaTeX.js
citekey = buildCiteKey(item, citekeys);
}
var citekey = buildCiteKey(item, extraFields, citekeys);
var extraFields = item.extra ? parseExtraFields(item.extra) : null;

This comment has been minimized.

Sign in to view
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.

Sign in to view
Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

Oops. Change submitted. That was unintentional.

This comment has been minimized.

Sign in to view
Copy link
@retorquere

retorquere Jan 30, 2019

Author Contributor

I've also bumped the dates.
BibTeX.js
@@ -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.

Sign in to view
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
BibLaTeX.js
@@ -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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
Copy link
@dstillman

dstillman Jan 30, 2019

Member

Citation Key: is good.
BibLaTeX.js
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.

Sign in to view
Copy link
@adam3smith

adam3smith Jan 30, 2019

Collaborator

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

This comment has been minimized.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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
variable assignment order
Loading status checks…
1d67d93
@retorquere

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

retorquere 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

@retorquere
CSL JSON id is useful for pandoc, TEI was already updated for .citati… 
…onKey but didn't have the extra-field parser
Loading status checks…
abaf012
@retorquere
bump timestamps
Loading status checks…
bf1f6c0
@retorquere
no empty citation key
Loading status checks…
b1c8f3b

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

1 check passed

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

adam3smith 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

@zuphilip
Revert #1814 for BibTeX.js
Loading status checks…
a73fcb0

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

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

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

@adam3smith
Re-introduce changes from #1814 (#1818) 
But this time without breaking stuff...
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4a58261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.