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

Adding a translator for idref.fr #1545

Merged
merged 7 commits into from Mar 12, 2018

Conversation

Projects
None yet
3 participants
@symac
Copy link
Contributor

symac commented Feb 3, 2018

Hello,
idref.fr is the website allowing to get information about authorities (identifiers) available in various French catalogs (mainly SUDOC and HAL for the moment). These linked catalogs already have existing translators so this translator is mainly calling the correct one based on the list of items existing on a page.

Currently the translator is working as expected (https://www.idref.fr/199676100 for example), but I have not been able to run the populated test, I imagine that this is related to the way I call selectItems but I have not been able to understand what the issue is), I hope the Zotero team will be able to fix this slight issue before merging.

Happy to update my code if any advice is given regarding things that would not be done according to Zotero's expectations.

symac added some commits Jan 12, 2018

Translator is working fine, except the test case I have not been able…
… to fill, maybe an issue with the code?
@adam3smith
Copy link
Collaborator

adam3smith left a comment

This is elegantly done -- I don't mind the tests failing. A couple of comments, mostly stylistic to keep this in line with our evolving style recommendations, one substantive comment.

Idref.js Outdated
"translatorID": "271ee1a5-da86-465b-b3a5-eafe7bd3c156",
"label": "Idref",
"creator": "Sylvain Machefert",
"target": "^https?://www\\.idref\\.fr/.*",

This comment has been minimized.

@adam3smith

adam3smith Feb 12, 2018

Collaborator

no need for the .* -- the target needs to match against the URL, not the other way around.

Idref.js Outdated
"priority": 100,
"inRepository": true,
"translatorType": 4,
"browserSupport": "g",

This comment has been minimized.

@adam3smith

adam3smith Feb 12, 2018

Collaborator

gcsibv

Idref.js Outdated
"browserSupport": "g",
"lastUpdated": "2018-02-03 09:39:41"
}

This comment has been minimized.

@adam3smith

adam3smith Feb 12, 2018

Collaborator

Add licenese

Idref.js Outdated
var resultsHref = ZU.xpath(doc, '//div[@id="perenne-references-docs"]/span[contains(@class, "detail_label")]/a/@href');

items = {};
for (var i in resultsTitle) {

This comment has been minimized.

@adam3smith

adam3smith Feb 12, 2018

Collaborator

iterate through arrays using either for (let i=0; i<resultsTitle.lenght; i++) or for (result of resultsTitle)

Idref.js Outdated
Z.debug("scraping : " + url);
var translator = Zotero.loadTranslator('web');

if (url.indexOf("archives-ouvertes")!=-1){

This comment has been minimized.

@adam3smith

adam3smith Feb 12, 2018

Collaborator

we've switched to using includes throughout for literal matches for readability (about same efficiency), so if (url.includes("archives-ouvertes")) etc.

Idref.js Outdated
href = resultsHref[i].textContent;
// We need to replace the http://www.sudoc.fr/XXXXXX links are they are redirects and aren't handled correctly from subtranslator
href = href.replace(/http:\/\/www\.sudoc\.fr\/(.*)$/, "http://www.sudoc.abes.fr/xslt/DB=2.1//SRCH?IKT=12&TRM=$1");
items[href] = resultsTitle[i].textContent;

This comment has been minimized.

@adam3smith

adam3smith Feb 12, 2018

Collaborator

Why not filter out the URLs that you know we won't be able to import here? Better to not offer those results than to fail on them.

@symac

This comment has been minimized.

Copy link
Contributor Author

symac commented Feb 12, 2018

Thanks @adam3smith . I have updated the file following your comments, should be better now. Tell me if you see anything else that could be improved.

Idref.js Outdated
Copyright (C) 2018 Sylvain Machefert - web@geobib.fr
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by

This comment has been minimized.

@zuphilip

zuphilip Feb 12, 2018

Collaborator

All Zotero translators should be licensed under AGPL (Affero), see https://github.com/zuphilip/translators/wiki/Common-code-blocks-for-translators for the relevant block.

This comment has been minimized.

@symac

symac Feb 12, 2018

Author Contributor

now fixed. I had copied another translator from the repo without having in mind this information regarding the license to be used. AGPL is also fine for me, thanks for the pointer.

@symac

This comment has been minimized.

Copy link
Contributor Author

symac commented Feb 23, 2018

@adam3smith @zuphilip what is the next step regarding this translator? I believe I have fixed the requests both of you did previously, or do I just need to wait (which is fine, no hurry, just want to be sure I am not missing something that I could do).

@adam3smith adam3smith merged commit aad2aa4 into zotero:master Mar 12, 2018

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Mar 12, 2018

Thanks!

@symac

This comment has been minimized.

Copy link
Contributor Author

symac commented Mar 15, 2018

@adam3smith can you confirm that it is working as expected for you? I have been told there were some issues and when testing, the translator is not showing in Firefox. If I try to visit this page in FF, nothing happens, if I visit the same page with Scaffold inside Zotero it is ok.

I have just resinstalled everything (Zotero, Scaffold, Firefox connector). Any idea ? don't know if you have access to debug reports but in case, I have just created one under reference D1823926315.

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Mar 15, 2018

Yes, there's something weird going on with detect. Reloading the page sometimes works for me but not always.

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.