process extra fields #3152

retorquere · 2023-09-27T09:20:24Z

Brings the extra-fields into the conversion process.

zoe-translates · 2023-10-10T06:21:19Z

Am I right in understanding that this is intended to bring the records in the Extra field into the item object as properties, so those extra fields with proper BibTeX counterparts could get a chance of getting into the BibTeX record rather than the "notes" text?

retorquere · 2023-10-10T06:24:23Z

That is correct.

retorquere · 2023-10-10T06:25:13Z

Although come to think of it, I'd also have to pick up the CSL fields in extra.

zoe-translates · 2023-10-10T06:30:18Z

Ah, I see, thanks. I wonder if there's more succinct way to achieve this goal, because I see a lot of duplication in the added code, which I fear might be prone to error if someone else is to work on them in the future. It would also be great to have more comments about what the code does. (To be fair there's too few comments to begin with in those translators).

retorquere · 2023-10-10T06:51:27Z

I wonder if there's more succinct way to achieve this goal

There is, I'll push that tonight.

retorquere · 2023-10-10T13:15:13Z

BTW personally I think this is a general feature across translators perhaps better handled in the framework rather than in these translators, but I can't oversee the implications of doing that.

retorquere · 2023-10-10T13:17:37Z

Also BTW the BibTeX.js translator has a fair number of pre-existing linting errors/warnings. I'd be happy to address those, but I think that is better handled in a separate PR, or reviewing would become hard.

zoe-translates · 2023-10-11T09:19:43Z

BibLaTeX.js

+		const rec = { raw: line };
+		const kv = line.match(/^([^:]+)\s*:\s*(.+)/);
+		if (kv) {
+			for (const field of (extra.field[field.field] || [])) {


This is an error (using the variable field before the declaration const field).

You're right -- sorry about that, that's now corrected.

zoe-translates · 2023-10-11T09:27:43Z

BibLaTeX.js

 	}
+};
+
+function parseExtraFields(item) {


I think I get what you mean here using the filter() to sieve through the extra fields. But this is going to be confusing because it may not be obvious that we're relying on the side effect of modifying item when calling this function. Please at least document this function (it's purpose and how it achieves that).

I've added comments, and renamed the function to better reflect what it does.

zoe-translates · 2023-10-12T05:03:03Z

BibTeX.js

@@ -1505,7 +1680,7 @@ function doExport() {
 			var extra = extraFieldsToString(extraFields); // Make sure we join exactly with what we split
 			if (extra) writeField("note", extra);


Here, you're writing what you have filtered from the original item's extra (extraFields) back into the Bib(La)TeX note field. I think this is why the generated export record has duplicate lines in the note. You'll likely want to replace this with if (item.extra) writeField("note", item.extra); outside the conditional if (extraFields)

This isn't my code though, that was already present before my change, so I must assume that that is intentional. Personally I don't see a good reason for them to be there, but I don't know what the use-case behind this was.

I don't think they are duplicate though? They're reproduced there which I didn't expect, but not twice.

They're reproduced there which I didn't expect, but not twice.

Have you tried resetting the translators in Zotero client after updating the code?

I did and I can reproduce the behaviour (duplicated fields from the extra).

The reason I suggest changing this part is that the code made sense before you implemented the filter-and-update step. It converted the extraFields array back into a string so that it would be dumped into the note field of the BibTeX output.

With your changes, the meaning of extraFields has changed. It is no longer a 1-1 copy of the item.extra record but converted to array; it now contains the relevant lines in item.extra already converted to "proper" fields (and removed from the original item.extra string). So the line

if (extra) writeField("note", extra);

dumps those already-converted fields into the note field. That's why this part of the code should be changed after your other changes.

Wouldn't resetting the translators reset them to the released version rather than my (still local) changes?

Hm, sorry, I wasn't clear. What is your procedure for testing the updated doExport() function? Do you do this in Scaffold (the do*/lightening button) or in the main client (saving exported files)?

Is scaffold really the only reliable way to test translators? I work from git and in the past having scaffold in the process was a bit of a pain.

Can I even add export tests to run in scaffold?

Alternatively, you can do live testing as follows: select an item in your library, go to Scaffold and click on the "do*/lightening" button, and inspect the output. This will execute the live code being edited even if it hasn't been saved.

I've ran this procedure and even though I'm of course still seeing extracted fields appear in the note field (which I'll change), I don't see them turning there up twice, which is what I thought you meant.

Translators are updated, fields are no longer written back into the note field.

It looks like scaffold trimmed (existing, not mine) lines with trailing whitespace, so the diff is larger than it used to be.

zoe-translates · 2023-10-12T05:07:47Z

BibLaTeX.js

-			rec.value = line.substr(splitAt + 1).trim();
-		}
-		fields.push(rec);
+const extra = {


Here, it's probably necessary to clearly mark this object as a "global" (top-level to the translator script) with ALL_CAPS and more descriptive name. There are too many variables/properties named "extra" or similar.

dstillman · 2023-10-17T15:55:18Z

BTW personally I think this is a general feature across translators perhaps better handled in the framework rather than in these translators, but I can't oversee the implications of doing that.

Yeah, sorry to come in late here, and thanks so much for working on this, but this really doesn't belong in individual translators. If we want to say that translators should automatically get converted fields from Extra, we should just do that in the core, where we already have CSL mappings and code to extract built-in and CSL fields from Extra. Code duplication aside, these mappings are bound to just get out of date.

retorquere · 2023-10-17T16:20:45Z

I'm with you on that. So, what now?

process extra fields

0862c2a

retorquere closed this Sep 27, 2023

retorquere reopened this Sep 27, 2023

separate out field aliases

951023d

retorquere added 3 commits October 10, 2023 13:00

less code, more config

a6131b0

more config, less code

84046ea

lint

050f09c

zoe-translates reviewed Oct 11, 2023

View reviewed changes

use before declaration

3247209

zoe-translates reviewed Oct 12, 2023

View reviewed changes

retorquere added 4 commits October 12, 2023 09:14

extra config in all-caps

52ff0ae

doc

8f2205c

rename

ace32be

don't write migrated fields back into the note

cb48b93

retorquere closed this by deleting the head repository Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process extra fields #3152

process extra fields #3152

retorquere commented Sep 27, 2023

zoe-translates commented Oct 10, 2023

retorquere commented Oct 10, 2023

retorquere commented Oct 10, 2023

zoe-translates commented Oct 10, 2023

retorquere commented Oct 10, 2023

retorquere commented Oct 10, 2023

retorquere commented Oct 10, 2023

zoe-translates Oct 11, 2023

retorquere Oct 11, 2023

zoe-translates Oct 11, 2023

retorquere Oct 12, 2023

zoe-translates Oct 12, 2023

retorquere Oct 12, 2023 •

edited

Loading

zoe-translates Oct 17, 2023

retorquere Oct 17, 2023

zoe-translates Oct 17, 2023 •

edited

Loading

retorquere Oct 17, 2023 •

edited

Loading

retorquere Oct 17, 2023

retorquere Oct 17, 2023

retorquere Oct 17, 2023

retorquere Oct 17, 2023

zoe-translates Oct 12, 2023

retorquere Oct 12, 2023

dstillman commented Oct 17, 2023

retorquere commented Oct 17, 2023

		@@ -1505,7 +1680,7 @@ function doExport() {
		var extra = extraFieldsToString(extraFields); // Make sure we join exactly with what we split
		if (extra) writeField("note", extra);

process extra fields #3152

process extra fields #3152

Conversation

retorquere commented Sep 27, 2023

zoe-translates commented Oct 10, 2023

retorquere commented Oct 10, 2023

retorquere commented Oct 10, 2023

zoe-translates commented Oct 10, 2023

retorquere commented Oct 10, 2023

retorquere commented Oct 10, 2023

retorquere commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retorquere Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoe-translates Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

retorquere Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstillman commented Oct 17, 2023

retorquere commented Oct 17, 2023

retorquere Oct 12, 2023 •

edited

Loading

zoe-translates Oct 17, 2023 •

edited

Loading

retorquere Oct 17, 2023 •

edited

Loading