Onboarding ropensci

[stijnvanhoey]

This PR tackles the suggestions provided by the ropensci onboarding review:

Maybe you could add a description to the get_token help file that states up front that you do not need to get a token right away. I was just randomly walking through the examples and tried that one towards the beginning. I then went back to the README to realize I didn't need to do it.

Good suggestion, adapted both the roxygen header of the get_token function as well as the Readme.md file.

Seems like this package would be a prime candidate for language translation. The little bit I looked into this awhile back, I think this SO post could get you started: https://stackoverflow.com/questions/29093673/how-to-translate-package-content

@peterdesmet what about this suggestion? I understand the idea, but as we are not controlling the error messages neither the dutch fields embedded in the waterinfo.be website, how could we properly provide this kind of translated version? Ideas/suggestions are welcome, but I would say to keep is as such.

I wonder if supported_frequencies() would make more sense to output a vector?

Good point, as such users can iterate over these frequencies or subset them. Adapted (and adapted test).

There are a lot of Factors returned, but a character vector now-and-then. Factors work great for the plots, maybe not so great for other purposes. I'd double-check that the columns that are coming back as Factors make sense to be factors.

Checked this and the main issue is the get_variables function, returning all factors instead of characters. I would keep it to characters by default as suggested. Adapted (and added test).

resolve_datasource...is this a function the general user will use? If not, maybe make it an internal function.

Available to the user, as it sometimes could be useful to have a quick check or control on the datasource (1 or 4 for the moment) themselves. hence, keep it public.

It might be useful to the user to offer a way to see or save the URL/GET query of the retrieved data. I'm not sure the best way, one could just be a message when retrieving, one way could be to add it to the attribute of the data.frame. (for the user, they may need to use it to cite the data, for you, you might need to use it when troubleshooting problems)

Great suggestion! I found a possible solution to add the URL of the response as a comment attribute to the returned data.frame. As such we can actually check the URL later... Updated roxygen documentation and tests as well.

[coveralls]

Coverage decreased (-0.2%) to 95.213% when pulling 618483990d09b4cebc1669458431e8a90e0ddfbf on onboarding-ropensci into 32b47f6ca8bad8db41f00e3c3b17f8d7d5608245 on master.

[stijnvanhoey]

@peterdesmet and @WillemMaetens if we agree on the responses in the PR, we can merge and I'll provide asap a response to https://github.com/ropensci/onboarding/issues/255#issuecomment-442282220

After further discussion with reviewer and if agreed (maybe after some other PRs), I'll bump minor version up (as we have small added features, nothing breaking) and release package again.

[stijnvanhoey]

• no additional recommendations from goodpractice package
• output R CMD CHECK:

0 errors ✔ | 0 warnings ✔ | 0 notes ✔

[peterdesmet]

Nice work @stijnvanhoey. Language translation: I agree with your rebuttal that it would be translating against a moving target that we do not control and would thus keep it out of scope.