Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd translator for new ACL Anthology site #1707
Conversation
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
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
nschneid
commented
Aug 10, 2018
Sounds good to me. Thanks! |
adam3smith
requested changes
Aug 12, 2018
Thanks! A couple of small questions/requests on this.
} 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.
Show comment
Hide comment
This comment has been minimized.
adam3smith
Aug 12, 2018
Collaborator
I think
ZU.xpath(doc, '//a[contains(., "BibTeX")]')[0].href
should work & is cleaner
adam3smith
Aug 12, 2018
Collaborator
I think
ZU.xpath(doc, '//a[contains(., "BibTeX")]')[0].href
should work & is cleaner
adam3smith
added
waiting
and removed
waiting
labels
Aug 12, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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?
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? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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.
Thanks -- everything looks good on your end, but I want to see if I can't get the test to work. |
adam3smith
removed
the
waiting
label
Aug 20, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
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!
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 |
GuyAglionby commentedAug 9, 2018
•
edited
Edited 1 time
-
-
GuyAglionby editedAug 9, 2018 (most recent)
GuyAglionby createdAug 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, causingitems
on line 88 to be empty andZotero.selectItems
on line 49 to error out withError: 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!