search

yihui / knitr

A general-purpose tool for dynamic report generation in R
https://yihui.org/knitr/
2.37k stars 874 forks source link

imgur_upload option? #2233

Closed jonthegeek closed 1 year ago

jonthegeek commented 1 year ago

Suggestion for improvement:

?knitr::imgur_upload says

Please register your own Imgur application to get your client ID; you can certainly use mine, but this ID is in the public domain so everyone has access to all images associated to it.

This would be much more feasible if imgur_upload() consulted an environment variable (and/or option) for the key.

Right now, when imgur_upload() is used inside reprex::reprex(), there's no way to pass a key.

I could PR this if you agree, but I'm not sure about testing nor whether there's already a pattern for this sort of thing in knitr.

cderv commented 1 year ago

I could PR this if you agree, but I'm not sure about testing nor whether there's already a pattern for this sort of thing in knitr.

Yes you can PR. We don't currently test it as part of the package - it is tested in knitr-examples

I don't think it needs further test just replacing the default key in the function with key = Sys.getenv("R_KNITR_IMGUR_KEY", <current default>).

Though I don't know if an option or an environment variable is preferred. @yihui do you have a preference ?

We should probably follow the pattern of xfun::tinify() which would lead to 

getOption("knitr.imgur.key", Sys.getenv("R_KNITR_IMGUR_KEY", <current default value for arg >))
jonthegeek commented 1 year ago

Environment variables are slightly more GHA-friendly, and I imagine that's a case where this would be used, so option > env > default makes sense to me!

I'll PR this in a sec...

github-actions[bot] commented 9 months ago

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.