Conversation
|
It looks like this is ready for me to try it out, review, merge, etc. Is that right @jmobrien? |
|
Yep. Should be ready to go as long as you agree. |
juliasilge
left a comment
juliasilge
left a comment
There was a problem hiding this comment.
This is looking great! 🙌 I've got just a couple of thoughts for changes.
| file = NULL, | ||
| save = TRUE |
There was a problem hiding this comment.
I think we've got a little bit of interacting arguments here between file and save. I think the best option is to make file = NULL do the same thing as the current save = FALSE. That means the default behavior (with file = NULL) would be to return the JSON content but not save it, but if someone provides a file then it will save it.
| #' default), or returned as output (\code{FALSE})? Even when \code{TRUE}, | ||
| #' downloaded JSON is returned invisibly. | ||
| #' | ||
| #' |
There was a problem hiding this comment.
Can we add an example? It would be a nice way to re-emphasize how to save as a file vs. only return the content. It will likely need to be wrapped so it doesn't execute like basically all our other examples.
|
Makes sense, easy enough to change. I'll add some examples, too. |
| url = description_url, | ||
| query = list(format = "qsf"), | ||
| as = "text", | ||
| encoding = "UTF-8" # Prevents a warning from guessing encoding |
There was a problem hiding this comment.
Can we instead make encoding an argument to this function so folks can pass in the encoding they need, as outlined in #336? We can still use UTF-8 as the argument default.
There was a problem hiding this comment.
See my reply in #336 about this--we can definitely do this if it's valuable, but I think we may have some other things to fix first before being able to properly decide on that.
2 participants
No description provided.