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

support other CRAN mirrors #1552

Merged
merged 8 commits into from Feb 27, 2018

Conversation

Projects
None yet
3 participants
@katrinleinweber
Copy link
Contributor

katrinleinweber commented Feb 8, 2018

Hi!

Since CRAN.RStudio.com is a very popular mirror, please consider expanding the translator to it. Related to #1487.

Also, would a minimal reg-ex to cover even other mirrors be advisable? cran\..+/web/packages might work.

Cheers :-)

katrinleinweber added some commits Feb 8, 2018

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Feb 8, 2018

Generally happy with this & extending it to other mirrors. Note you'll have to bump the timestamp for updates to propagate (Scaffold, which we recommend, would do that automatically)

@@ -2,7 +2,7 @@
"translatorID": "24a10ebf-ada1-4b8d-8f76-5a29e24d3e78",
"label": "R-Packages",
"creator": "Sebastian Karcher",
"target": "r-project\\.org/web/packages/(.+/index\\.html|available_packages_by_(name|date)\\.html)",
"target": "(r-project\\.org|rstudio\\.com)/web/packages/(.+(/index\\.html)?|available_packages_by_(name|date)\\.html)",

This comment has been minimized.

@adam3smith

adam3smith Feb 8, 2018

Collaborator

once you make index.html optional, you can just delete the entire regex after packages: .+(/index\\.html)? literally matches anything.

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Feb 8, 2018

What's the form of the other mirrors? I thought we already covered those?

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 8, 2018

Oh! That was an unwarranted assumption it seems. I was thinking of https://www.r-pkg.org/pkg/ggplot2 but they seem to have an entirely different site structure.

katrinleinweber added some commits Feb 8, 2018

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 8, 2018

@katrinleinweber katrinleinweber changed the title expand to CRAN mirror support other CRAN mirrors Feb 8, 2018

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 8, 2018

I feel bad duplicating the test code so much. Does Scaffold support extracting multiple URLs and comparing each result against the same items set? Or against each other?

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Feb 8, 2018

Don't worry about it -- we only very rarely test different sites with the same expected result, so not worth implementing dedicated functionality for that scenario, and the size of these tests is just completely negligible (and it's not like it's code that actually needs to be updated/maintained by humans)

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 8, 2018

I delivered it with my bare hands, but it's good to know I don't have to take care of it. 🤣

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 12, 2018

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Feb 18, 2018

I rewrote it w/o the FW and update some things. Please have a look. Should be fine for merge.

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 19, 2018

Manual test imports look fine for me (Zotero 5.0.35.1 & Firefox 52.6.0). Thank you :-)

"creatorType": "author"
},
{
"firstName": "R. Core",

This comment has been minimized.

@adam3smith

adam3smith Feb 27, 2018

Collaborator

we should recognize the R Core Team and turn it into an institutional author. Presumably that'll show up on a fair number of packages, so should be worth it.

This comment has been minimized.

@zuphilip

zuphilip Feb 27, 2018

Collaborator

Done in the new version.

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 27, 2018

Thank you :-)

@zuphilip zuphilip merged commit 24a2462 into zotero:master Feb 27, 2018

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Feb 27, 2018

Okay, it is merged now. Thank you all for the contributions here!

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Feb 27, 2018

@katrinleinweber Can we close the corresponding issue or is there something still open? (I haven't followed the conversion there closely.)

@katrinleinweber

This comment has been minimized.

Copy link
Contributor Author

katrinleinweber commented Feb 27, 2018

#1487 is "related", because it deals with CRAN iFrames. It's not completely corresponding to this PR, so it should IMHO stay open.

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.