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

Add China/Asia On Demand translator #1737

Merged
merged 2 commits into from Sep 9, 2018

Conversation

@GuyAglionby
Copy link
Contributor

commented Sep 8, 2018

Addresses #1716.

The second test case has the university names as authors, as this is what is indicated on the website. Unfortunately I can't think of a good way of detecting these and removing them, given the range of things (especially wrt. spacing) seen in the author field (e.g. one, two).

Thanks!

@adam3smith
Copy link
Collaborator

left a comment

Thanks for taking this! Two small things (the first one is really more cosmetic).

CAOD.js Outdated
"translatorID": "d2a9e388-5b79-403a-b4ec-e7099ca1bb7f",
"label": "CAOD",
"creator": "Guy Aglionby",
"target": "^https?://caod\\.oriprobe\\.com/articles/[^#]+",

This comment has been minimized.

Copy link
@adam3smith

adam3smith Sep 8, 2018

Collaborator

remember you don't need the target to capture the whole URL, it simply runs target.test(url) so the [^#]+ doesn't do anything

*/

function detectWeb(doc, url) {
if (url.includes('articles/found.htm?')) {

This comment has been minimized.

Copy link
@adam3smith

adam3smith Sep 8, 2018

Collaborator

this needs to be more restrictive, e.g. gives a false positive on empty searches such as http://caod.oriprobe.com/articles/found.htm?keyword=rtryiy&package=&key_author=&key_qkname=&key_year=&key_volumn=&key_issue=

that's why we have getSearchResults in the templates -- it's pretty much always a good idea to match doWeb and detectWeb to avoid false positive in the latter.

@GuyAglionby

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2018

Thanks for the feedback and the heads up re. getSearchResults -- made both changes :)

@adam3smith adam3smith merged commit 5d260e6 into zotero:master Sep 9, 2018

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.