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

imgur_upload option? #2233

Closed
jonthegeek opened this issue Feb 24, 2023 · 3 comments · Fixed by #2235
Closed

imgur_upload option? #2233

jonthegeek opened this issue Feb 24, 2023 · 3 comments · Fixed by #2235

Comments

@jonthegeek
Copy link
Contributor

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
Copy link
Collaborator

cderv commented Feb 27, 2023

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
Copy link
Contributor Author

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...

jonthegeek added a commit to jonthegeek/knitr that referenced this issue Feb 27, 2023
Closes yihui#2233.

I implemented it like xfun::tinify(), but I'm not sure if it makes sense to involve opts_knit here.
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants