Inconsistent return types in package functions

[nickdickinson]

At the moment, there are few functions that are not behaving the same as others. When there is an error, they do not return an error but rather a specially formatted object or a NULL. This makes it difficult to have a consistent and predictable pattern for each function call in the package and may also confuse users. I'm opening this issue to be able to document these in one place and so we can discuss.

As I am writing tests for the package functions, I've noted two so far:

• getDatabaseUsers() returns NULL if there is no user (http status == 404). However, one should expect that if the user is being fetched by ID then you know the ID and if it does not work, you would expect an error rather than a NULL value like with the other package functions.
• addDatabaseUser() returns a custom object that is contains both a boolean for whether the user was added returnvalue$added = TRUE and returnvalue$user as a list if the user was added. If there is an http status == 400, it gives returns returnvalue$added = FALSE and returnvalue$error with the response object and finally with any other http status it throws an error.
• most other functions will just throw an error if there is not a 200 status and return the API object otherwise.

Ideas:

1. make the API objects more user friendly and a pleasure to code with so we do not need custom return values
2. make helper functions to work with the API objects
3. make a vignette on dealing with errors in a good way (logging/logic/etc)
4. create more consistency and softly / slowly deprecate the return values that are more inconsistent

[Ryo-N7]

here is an example of the output from addDatabaseUser():

i actually quite like this output because it's easy to ingest and reformat into a dataframe for error handling purposes

[nickdickinson]

It is definitely much nicer to get an object! It would be better for all functions return the API response object (the user) directly on success. For POST or PUT requests, it would at least return the input given to the function (formId) if the API does not return anything in the tidyverse manner.

BUT it would be cleaner to handle errors of all functions in a consistent manner so that all the package functions act in a similar way and give the user/coder a single way to handle any number of error types. On error, I would propose to simply throw the error condition on failure. This can work across all types of functions regardless of whether they are get, post, put and regardless of the nature of the error.

In the case of addDatabaseUser(), if the server responds with the HTTP status 400, it returns added=FALSE, otherwise it will still throw an error if HTTP status is != 200 or 400, such as a temporary server 5XX error. So you still need to put a try/tryCatch around it with some handling as well as some logic checking the return value for an error when adding a user. Assuming the user needs to know why the error happened, I think it is better to let them use the codes to change the handling rather than hardcoding it so users then need two different kinds of error handling. I think it would be helpful to document relevant codes in the Roxygen!

Logging of error conditions and a tryCatch/tryCatchLog type construction should enable you to do any error handling required and check the status of each error if necessary and inspect the error object.

[akbertram]

@nickdickinson can this be closed?

[nickdickinson]

The intention was to keep a list of all the R package functions that don't use the standard pattern of providing the same response as the server and throwing an error when there is an API error. This is still the case with addDatabaseUser(). They mostly also do not use the rest.R functions to talk to the API as well, which means they will not support errors in the same way and will need to be maintained separately.

[Ryo-N7]

updateUserRole() gives no output, not even any API response messages so it's a bit uninformative of whether it was successful or not (i imagine if i do something wrong it'll give me an error but haven't come across that yet)