New imgur_upload with xml2 and httr #1433
Conversation
There was a problem hiding this comment.
Note that RCurl is also used in knitr::image_uri2 for RCurl::base64Encode. httr has a base64url but not exported. What should we do to remove RCurl suggest dependency to RCurl ?
The image_uri2() function can be removed. It is not actually used in this package. I'll take care of it.
XML package is used is called inst/examples/knitr-themes.Rnw. I think we can remove the suggests dependency but not sure.
This Rnw document is not compiled during R CMD build / check, and you can ignore it.
One thing: There is a imguR package with a upload_imgur function. I do not know if it is better to have a imgur_upload function specific to knitr or if it may be a good idea to use a dependency toward imguR package and import the imgur_upload.
For a simple function like imgur_upload(), I'd rather not introduce a new dependency.
BTW, could you sign the contributor agreement? http://www.clahub.com/agreements/yihui/knitr
R/utils-upload.R
Outdated
| if (!has_package("xml2") || !has_package("httr")) { | ||
| stop("Pkg xml2 and httr needed for uploading file to imgur. Please install them or do not upload files.", | ||
| call. = FALSE) | ||
| } |
There was a problem hiding this comment.
I'd remove this if statement, and let httr:: / xml2:: fail directly later.
There was a problem hiding this comment.
I wanted to apply some good practice. Maybe it is not.
R/utils-upload.R
Outdated
| if (is.null(encoding <- info_media$params$charset)) { | ||
| message("No encoding in response : defaulting to UTF-8") | ||
| encoding <- "utf-8" | ||
| } |
There was a problem hiding this comment.
Is it safe to just assume UTF-8? Six lines of code (49-54) to find the encoding is probably not worth it in my eyes. This function does not need to be "bulletproof" :)
There was a problem hiding this comment.
I think we can assume that it is UTF-8. I did not found the info on imgurAPI but should be safe. You're right - no need to be too "bulletproof" 😄
man/imgur_upload.Rd
Outdated
| @@ -18,8 +18,8 @@ A character string of the link to the image; this string carries an | |||
| file; see Imgur API in the references. | |||
| } | |||
| \description{ | |||
| This function uses the \pkg{RCurl} package to upload a image to | |||
There was a problem hiding this comment.
Could you leave out the changes in Rd? https://github.com/yihui/knitr/blob/master/.github/CONTRIBUTING.md#roxygen2-documentation
There was a problem hiding this comment.
Sorry for that. I should have read more closely your documentation. I reverted the changes.
|
I took care of your review comments. Thanks. There is no test for library(ggplot2)
ggplot(mtcars, aes(mpg, wt)) + geom_point()
ggsave("mtcars.png")
link <- imgur_upload("mtcars.png")
# should open the image on a browser
browseURL(link)
# should give XML attributes
str(attributes(link),1)Not sure how to implement test with image upload... As it is not there already I did not create a test. |
|
Don't worry about the test. This function is called in https://github.com/yihui/knitr-examples/blame/master/010-upload.Rmd and the knitr-examples repo is automatically compiled on Travis. As long as this PR can pass the check on Travis, it should be fine. BTW, I don't see your name in https://github.com/yihui/knitr/blob/master/DESCRIPTION. Could you add yourself as a contributor there? |
|
Yep! I was just thinking about that. I forgot this file. It is not a habit yet as not every project ask you that. One more thing, I did not remove XML and RCurl as dependencies in DESCRIPTION. I leave that to you. |
|
Sounds good. No hurry. |
|
Done. I had myself as a contributor. It seems to be an alphabetical order of contributor in |
Hi @yihui,
Following a PS call in your recent blog post, I modified
imgur_uploadto usehttrandxml2instead ofRCurlandXMLNote that
RCurlis also used inknitr::image_uri2forRCurl::base64Encode.httrhas abase64urlbut not exported. What should we do to removeRCurlsuggest dependency to RCurl ?XMLpackage is used is calledinst/examples/knitr-themes.Rnw. I think we can remove the suggests dependency but not sure.One thing: There is a imguR package with a
upload_imgurfunction. I do not know if it is better to have aimgur_uploadfunction specific toknitror if it may be a good idea to use a dependency towardimguRpackage and import theimgur_upload.What do you think of all this ?
Does it help ?
This change is