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

[Zenodo] Prevent mis-importing software as report #1578

Merged
merged 10 commits into from
Mar 9, 2018

Conversation

katrinleinweber
Copy link
Contributor

@katrinleinweber katrinleinweber commented Mar 7, 2018

As discussed in the forum this ensures that Software items on Zenodo are imported as computerProgram, even though along the way, Zenodo is offering a non-CSL item type

Potential/Remaining isues to discuss

  • Scaffold imports the full author names as lastName. This could occur in https://github.com/zotero/translators/blame/master/Zenodo.js#L172-L180, but I'm not sure whether or how to adjust that. Or, should I adjust the new test case to expect lastName and firstName normally?
  • Would it be helpful, if Zenodo stopped putting a clearly mis-matching CSL type in the JSON that Zotero parses, and instead send none? If yes, could we then omit both the "fix" and this "undo"?
  • Scaffold added - and + in the doWeb test output (near the bottom in the diff). Not sure what those mean, but I guess both symbols need to be removed from the JSON block.

@adam3smith
Copy link
Collaborator

How are you generating the tests in Scaffold? This is fine now, but to save you the work going forward, if you actually generate the test using the Scaffold testing panel, you won't get the + and - (which are from a diff display Scaffold shows).

Zenodo.js Outdated
@@ -195,6 +195,11 @@ function scrape(doc, url) {

if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would happen if we replace this whole thing, including l. 197 by item.itemType = detectWeb(doc, url)
That seems like a cleaner solution, provided our detect function is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the var zoteroType block be removed as well, then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, yes

@adam3smith
Copy link
Collaborator

The Zenodo item types in the CSL are fine -- they are as good as it gets for e.g. automated citation generation directly from the CSL and Zotero can customize at will in the import.

For the names, we can't really parse these correctly without breaking institutional names since they're entered incorrectly in Zenodo, but we can switch fieldMode to true along the lines for

for (let i =0; i< item.creators.length; i++) {
  if (!item.creators[i].firstName) {
    item.creators[i].fieldMode = true;
  }
}

This makes them easier to fix manually.

Zenodo.js Outdated
@@ -175,6 +175,7 @@ function scrape(doc, url) {
if (!item.creators[i].firstName && item.creators[i].lastName.indexOf(",")!=-1) {
item.creators[i].firstName = item.creators[i].lastName.replace(/.+?,\s*/, "");
item.creators[i].lastName = item.creators[i].lastName.replace(/,.+/, "");
item.creators[i].fieldMode = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? No changes in the test cases??

I think what we want is rather want is:

if (!item.creators[i].firstName) {
	if (item.creators[i].lastName.indexOf(",")!=-1) {
		item.creators[i].firstName = item.creators[i].lastName.replace(/.+?,\s*/, "");
		item.creators[i].lastName = item.creators[i].lastName.replace(/,.+/, "");
	} else {
		item.creators[i].fieldMode = true;
	}
}


if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we delete the first introduction of the variable type as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean L42 or L133? Removing the latter indeed doesn't break the tests :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the latter one seems not necessarily anymore. We certainly still need the first one.

Zenodo.js Outdated
if (item.itemType == "document" && zoteroType[type]) {
item.itemType = zoteroType[type];
}
item.itemType = detectWeb(doc, url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ; at the end.

Zenodo.js Outdated
"lastName": "Carl Boettiger"
"firstName": ""
"creatorType": "author"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rerun the tests should yield some fieldMode fields.

@zuphilip zuphilip merged commit 5fa9a68 into zotero:master Mar 9, 2018
@zuphilip
Copy link
Contributor

zuphilip commented Mar 9, 2018

🎉 Thank you @katrinleinweber !

@katrinleinweber
Copy link
Contributor Author

You're welcome, and thanks for merging :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants