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

Fix Galegroup #1750

Merged
merged 9 commits into from Oct 8, 2018
+319 −79

Conversation

Reviewers

@zuphilip zuphilip

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@adam3smith @zuphilip
@adam3smith
Copy link
Collaborator

commented Sep 24, 2018

This works for all standard Gale databases and is greatly simplified. A
small subset of Gale looks completely different & it makes little sense
to put/keep those in the same translator:

  • NewsVault
  • Archives Unbound
  • The Times Digital Archive
  • The Times Literary Supplement Historical Archive, 1902-2013
  • Eighteenth Century Collections Online

There already is a separate translator for GaleGDC.

No tests -- while there are permalinks, those include the university username...
@adam3smith
Fix Galegroup 
This works for all standard Gale databases and is greatly simplified. A 
small subset of Gale looks completely different & it makes little sense 
to put/keep those in the same translator:
- NewsVault
- Archives Unbound
- The Times Digital Archive
- The Times Literary Supplement Historical Archive, 1902-2013
- Eighteenth Century Collections Online

There already is a separate translator for GaleGDC
Loading status checks…
24452d8

@adam3smith adam3smith requested a review from zuphilip Sep 24, 2018

@adam3smith adam3smith changed the title Fix Galegroup [WIP; don't merge] Fix Galegroup Sep 24, 2018

adam3smith added some commits Sep 25, 2018

@adam3smith
Gale: Split new & old databases
Loading status checks…
6ecbe7f
@adam3smith
Nits
Loading status checks…
b4873d6
@adam3smith
No tests in old Gale
Loading status checks…
61f7450

@adam3smith adam3smith changed the title [WIP; don't merge] Fix Galegroup Fix Galegroup Sep 25, 2018

@adam3smith
Fix UUID
Loading status checks…
af512af
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

commented Sep 25, 2018

@zuphilip This is now actually ready to check out. Two separate translators, one simple handling the unified modern databases, one adapting the old one for the legacy ones.
@zuphilip

zuphilip reviewed Oct 7, 2018
View changes

Copy link
Collaborator

left a comment

The new translator looks great and I have only some small comments, which should be easy to fix.
Galegroup.js Outdated
}

processPage(doc, url);
}
}
/** BEGIN TEST CASES **/
}/** BEGIN TEST CASES **/

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Let us keep that newline before the (empty) test section. This makes it much easier to parse later...
Galegroup.js Outdated
@@ -2,20 +2,20 @@
"translatorID": "4ea89035-3dc4-4ae3-b22d-726bc0d83a64",
"label": "Galegroup",
"creator": "Sebastian Karcher and Aurimas Vinckevicius",
"target": "^https?://(find|go)\\.galegroup\\.com",
"target": "^https?://(find\\.galegroup\\.com/|go\\.galegroup\\.com/gdsc)",
"minVersion": "3.0",
"maxVersion": "",
"priority": 100,
"inRepository": true,
"translatorType": 4,
"browserSupport": "gcsib",

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Add all of the options here
Show resolved Hide resolved Galegroup.js
Galegroup.js Outdated
return results;
}

//Academic OneFile
//Academic ASAP
// Ecco

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

1 space after //
Galegroup.js Outdated
@@ -196,6 +200,30 @@ function composeAttachmentTDA(doc, url) {
return composeAttachmentGNV(doc, url);
}

function composeAttachmentEcco(doc, url) {
// This is the code for a post request for the PDF, not sure if this is possible, though?

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Was this the question Dan answered somewhere? I would rather not have these large code block in the comments as following here.
Gale Databases.js Outdated
var productName = attr(doc, 'input.citationToolsData', 'data-productname');

var documentData = '{"docId":"' + docId +'","documentUrl":"' + documentUrl + '","productName":"' + productName + '"}';
var post = "citationFormat=RIS&documentData=" +encodeURIComponent(documentData).replace(/%20/g, "+");

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Add space after +
Gale Databases.js Outdated

function parseRis(text, attachment) {
text = text.trim();
//gale puts issue numbers in M1

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Add space after //. Also in the following lines.
Gale Databases.js Outdated

var documentData = '{"docId":"' + docId +'","documentUrl":"' + documentUrl + '","productName":"' + productName + '"}';
var post = "citationFormat=RIS&documentData=" +encodeURIComponent(documentData).replace(/%20/g, "+");
var attachment = composeAttachment(doc, url);

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018 •
edited

Collaborator

Any reason that you do the attachment thing here in the scrape function and then pass it to the scrapeRIS function rather than doing it directly in the scrapeRIS function?
Gale Databases.js Outdated
//gale puts issue numbers in M1
text = text.replace(/M1\s*\-/g, "IS -");
//L2 is probably meant to be UR, but we can ignore it altogether
text = text.replace(/^L2\s+-.+\n/gm, '');

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Can we restrict the content a little further here which we can safely delete? Is it always an URL? I fear the .+ regex part here a little...

This comment has been minimized.

Sign in to view
Copy link
@adam3smith

adam3smith Oct 7, 2018

Author Collaborator

This just remove the L2 line entirely and is meant to. .+ doesn't match new lines, so there's really no danger here.

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Okay, but we could also be more specific, e.g. something like

text = text.replace(/^L2\s+-\s+https?:\/\/\S+\s*$/gm, '');

Then, if the field L2 is also used as something else then URLs, we still consider it.
Gale Databases.js Outdated
translator.setTranslator("32d59d2d-b65a-4da4-b0a3-bdd3cfb979e7");
translator.setString(text);
translator.setHandler("itemDone", function (obj, item) {
if(attachment) item.attachments.push(attachment);

This comment has been minimized.

Sign in to view
Copy link
@zuphilip

zuphilip Oct 7, 2018

Collaborator

Space after if

adam3smith added some commits Oct 7, 2018

@adam3smith
Update Gale Databases.js
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…
ff309b5
@adam3smith
more typos
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…
133ec14
@adam3smith
Gale updates after reviews
Loading status checks…
0115651
@adam3smith
More space typos
Loading status checks…
a9c1bea
@adam3smith

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

commented Oct 8, 2018

Thanks! I addressed all your other comments except the L2 one. L2 maps to attachment/html in Zotero, i.e. it's looking for a local attachment and doesn't make a lot of sense here. I've also failed to find examples of this in a couple of translators, so this might be a moot point, but I'd like to keep the line in just in case.

@zuphilip zuphilip merged commit 449675b into zotero:master Oct 8, 2018
1 check passed

1 check passed

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Oct 8, 2018

Great! Merged now 🎉 ! Thank you @adam3smith !
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.