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

Add translator for new ACL Anthology site #1707

Merged
merged 3 commits into from Aug 25, 2018

Conversation

Projects
None yet
3 participants
@GuyAglionby
Contributor

GuyAglionby commented Aug 9, 2018

New translator for https://aclanthology.coli.uni-saarland.de/ (cf #1702)

One small problem with the test case https://aclanthology.coli.uni-saarland.de/volumes/computational-linguistics-volume-44-issue-1-april-2018 . Scaffold happily imports it automatically, but when actually running it the xpath on line 74 erroneously returns nothing, causing items on line 88 to be empty and Zotero.selectItems on line 49 to error out with Error: Translator called select items with no items. This happens despite the test case working in manual testing. Not too sure what might cause this; any help appreciated!

Also worth noting that the PDFs of 2018 papers are still stored on the old domain (e.g. paper overview with corresponding PDF), so trying to save from the PDF is dealt with by the old translator (ACLWeb.js). This translator fails on the '18 papers, as they are not actually indexed on the old site. As a result, I suggest a small change to ACLWeb.js to pull BibTeX information from the new site, rather than scraping it from the old. Happy to do this if it sounds reasonable to you.

Thanks!

@GuyAglionby GuyAglionby changed the title from Add translator for new ACL Anthology site (solves #1702) to Add translator for new ACL Anthology site Aug 9, 2018

@nschneid

This comment has been minimized.

Show comment
Hide comment
@nschneid

nschneid Aug 10, 2018

Sounds good to me. Thanks!

nschneid commented Aug 10, 2018

Sounds good to me. Thanks!

@adam3smith

Thanks! A couple of small questions/requests on this.

Show outdated Hide outdated ACLAnthology.js
} else if(url.endsWith('.bib')) {
scrapeBibtex(ZU.xpath(doc, '//pre')[0].innerHTML);
} else {
let partialBibtexUrl = ZU.xpath(doc, '//a[contains(., "BibTeX")]/@href')[0].value;

This comment has been minimized.

@adam3smith

adam3smith Aug 12, 2018

Collaborator

I think
ZU.xpath(doc, '//a[contains(., "BibTeX")]')[0].href should work & is cleaner

@adam3smith

adam3smith Aug 12, 2018

Collaborator

I think
ZU.xpath(doc, '//a[contains(., "BibTeX")]')[0].href should work & is cleaner

Show outdated Hide outdated ACLAnthology.js

@adam3smith adam3smith added waiting and removed waiting labels Aug 12, 2018

@GuyAglionby

This comment has been minimized.

Show comment
Hide comment
@GuyAglionby

GuyAglionby Aug 12, 2018

Contributor

Thanks for the review! I display the authors + the paper ID when selecting multiple as this is what we do with the old translator. Happy to change to paper title exclusively for consistency with the rest of the translators.

As for the failing test, I can go ahead and remove it and instead add the link as a comment in the relevant part of the code, if this sounds reasonable?

Contributor

GuyAglionby commented Aug 12, 2018

Thanks for the review! I display the authors + the paper ID when selecting multiple as this is what we do with the old translator. Happy to change to paper title exclusively for consistency with the rest of the translators.

As for the failing test, I can go ahead and remove it and instead add the link as a comment in the relevant part of the code, if this sounds reasonable?

@adam3smith

This comment has been minimized.

Show comment
Hide comment
@adam3smith

adam3smith Aug 20, 2018

Collaborator

Thanks -- everything looks good on your end, but I want to see if I can't get the test to work.

Collaborator

adam3smith commented Aug 20, 2018

Thanks -- everything looks good on your end, but I want to see if I can't get the test to work.

@adam3smith adam3smith removed the waiting label Aug 20, 2018

@adam3smith

This comment has been minimized.

Show comment
Hide comment
@adam3smith

adam3smith Aug 25, 2018

Collaborator

OK, I removed the test and added as comment for now. The discussion on zotero-dev for the functionality that I think would get the test to work is here: https://groups.google.com/forum/#!topic/zotero-dev/Dy01PtaZ53E
Thanks for working on this!

Collaborator

adam3smith commented Aug 25, 2018

OK, I removed the test and added as comment for now. The discussion on zotero-dev for the functionality that I think would get the test to work is here: https://groups.google.com/forum/#!topic/zotero-dev/Dy01PtaZ53E
Thanks for working on this!

@adam3smith adam3smith merged commit e05e998 into zotero:master Aug 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment