translators icon indicating copy to clipboard operation
translators copied to clipboard

New translator for Cowles Foundation publications

Open vcarret opened this issue 5 years ago • 11 comments

A translator for the documents on the Cowles Foundation website (https://cowles.yale.edu/). The translator works on the authors' pages and on the different lists of publications (for monographs, discussion papers and published articles), and gets pdfs when available. I tried to follow the guidelines at https://www.zotero.org/support/dev/translators.

vcarret avatar Nov 21 '19 11:11 vcarret

Thanks! Some suggestions to clean up the code a bit & make it more robust

Thank you for your quick feedback! This is my first translator and I'm not used to Github so I apologize in advance for any silly mistakes

vcarret avatar Nov 22 '19 10:11 vcarret

This all looks very good now thanks (and apologies for the delay). Two remaining issues:

  1. The root that you specify -- that shouldn't be necessary. Zotero can handle relative links and they're more robust. Could you test if all of this still works entirely without root and then remove it? (If you're used to other webscraping tools e.g. in R or python, they tend to behave less like a browser and break with relative links; Zotero doesn't)

  2. Please use the minimized version of text() and attr() as e.g. here: https://github.com/zotero/translators/blob/master/Emerald%20Insight.js#L37 use all three lines -- the first one identifies, the 2nd disables linting, the 3rd is the minimized code.

adam3smith avatar Dec 05 '19 03:12 adam3smith

I am not sure how to do without the root part because, if I understand it correctly, Zotero inserts the current url argument if I don't specify it; while it would work on the items pages or the list of publications, it would break eg on an authors' page where the current url does not match the beginning of an item's url (for example: you're on this page https://cowles.yale.edu/author/herbert-e-scarf, and select an item to import to zotero. The current url used is https://cowles.yale.edu/author and if I just append the item url for "/cfdp-2212" it throws a 404 because the actual beginning should be https://cowles.yale.edu/publications/cfdp). Same goes for the PDF, as what is scraped is an address starting at the root of the site; if I delete var root it sends a GET to the current URL (eg. https://cowles.yale.edu/publications/archives/cfm) + the scraped address whereas it should only send a GET to https://cowles.yale.edu/ + the scraped address...

Maybe I missed something because I did not take the time yet to see how Zotero connector works please tell me if you think I could do it using only relative links

vcarret avatar Dec 05 '19 17:12 vcarret

Did you test? Relative links always refer to the host, i.e what you call the root and that's what Zotero uses (or certainly should use). In other words root + "/" + variable is essentially identical to "/" + variable but will behave better e.g. if URL proxies are involved or with http/https differences

adam3smith avatar Dec 05 '19 21:12 adam3smith

Sorry I was confused it seems I got that error by testing without the initial /, in that case apparently Zotero appends the entire current URL and not just the host. It seems to work now with the initial /

vcarret avatar Dec 05 '19 22:12 vcarret

Great thanks. Just FYI

I got that error by testing without the initial /, in that case apparently Zotero appends the entire current URL and not just the host. It seems to work now with the initial /

This is just how relative links work, e.g. that's how a browser would behave on a website.

adam3smith avatar Dec 06 '19 16:12 adam3smith

@adam3smith do you need any more changes on this translator?

vcarret avatar Jan 17 '20 21:01 vcarret

Hey @AbeJellinek thanks for your effort to go down the list of opened pull requests!

I did the changes you requested you were right about the duplicated logic :)

Checks are failing but the tests worked on my computer; I am a bit rusty about the linting process and maybe it changed since the last time I committed to this repo do you mind checking why it failed? I can squash the commits if you want

vcarret avatar Jul 16 '21 15:07 vcarret

No need to squash manually! I can do that when merging. The easiest way to deal with linting is like this:

$ git pull origin
$ git rebase origin/master
$ npm run lint -- "Cowles Foundation.js" --fix

Then commit and force push.

npm run lint will complain about a few things that it can't fix automatically. In this case it seems to be that you don't declare author before using it on line 119 and line 134. You can just add a let to the beginning of each of those lines.

AbeJellinek avatar Jul 16 '21 16:07 AbeJellinek

Oh my God, I'm sorry, I wrote git pull origin but meant git fetch origin. That's where all the irrelevant commits are coming from. A rebase on origin/master should remove them.

AbeJellinek avatar Jul 16 '21 18:07 AbeJellinek

Ok sorry I did not check what I was doing but now I reset everything into the first commit, although I still don't understand what is this failing error: "fatal: ambiguous argument '577948faa947649ea40185e4730f0391f0b98535^^!': unknown revision or path not in the working tree."

vcarret avatar Jul 16 '21 18:07 vcarret