Add all relevant API parameters as function params for user interface improvements about fredr HOT 12 CLOSED

I've started work below, you can reference ?fredr_series for the messiness I'm talking about.

On further thinking about this, I think including all of the relevant function parameters + their documentation in each function is a bit overkill and makes the package a bit messy. Rather, we could use a combination of tricks I used in furrr (from Hadley actually) + some tricks from the documentation of ggplot2 and the aes() options.

How do you feel about each function only having the core parameters as arguments, then having a generic fredr_options() function that returns a list and has as arguments all the possible parameters for the API. Then the documentation of the parameters could be in 1 place, that fredr_options() function's help doc.

To prevent confusion, the help doc of fredr_series() could have a section Optional Parameters that just lists the parameters that it accepts from fredr_options(). This is like ggplot2, see Aesthetics from ?ggplot2::geom_line. If the user sets any that shouldn't be used, it could just ignore them with a warning.

It would look like this.

# core options of maybe series_id, observation_start, observation_end
fredr_series(series_id, observation_start, observation_end, options = fredr_options())

# core options of maybe search_text, search_type
fredr_series_search(search_text, search_type, options = fredr_options())

fredr_series("my_series", Sys.Date() - 10, Sys.Date(), options = fredr_options(frequency = "m", limit = 10)

from fredr.

That way, the fredr_series() help page is much cleaner and only states the core of what it does, but still provides the flexibility to specify all those other options. It also makes it easier on us as we don't have to go through each function's endpoint and add all the parameters as arguments to the function and make sure they pass through correctly.

from fredr.

I do like the idea of required params + fredr_options() in each function, with fredr_options() documenting common optional parameters for each function. However, I'd prefer to only include parameters that are required and do not have a default. So for your example with fredr_series(), I don't feel the need to include observation_start and observation_end as required params. I don't mind documenting them in the package but I often find myself running fredr_series() without specifying observation dates. I'd also like to avoid hard coding the API defaults. What do you think?

from fredr.

Makes sense, I will:

• Only make required parameters explicit arguments
• Change the hard coded API defaults to instead be NULL, ensuring that the user knows by reading the docs what the default is

from fredr.

Also, in terms of exposing all endpoints, I was going to suggest:

fredr_series -> fredr_series_observations

fredr_search -> fredr_series_search

fredr_search_id -> just part of fredr_search, use the search_type param to switch between full text and id.

fredr_search_rel_tags -> fredr_search_related_tags

1. This is consistent with the endpoints
2. This allows us to add fredr_series that hits the fred/series endpoint

I just think it makes sense to stay really consistent with the endpoints for users that cross check between them. Let me know if you think otherwise! It is a mouthful to use fredr_series_observations rather than fredr_series, but I think some examples in the README would clear that up for users pretty quickly.

from fredr.

Hmm, running into times when the same parameter is documented inconsistently between endpoints. For instance, limit is 0-100000 for series/observations but is 0-1000 for tags. Kind of hurts the ability to document them all in 1 place.

I think we can get away with kind of generic definitions of parameters, and then in the Details section of fredr_options state that you should consult the docs for exact limits and definitions in the context of the specific function. I was planning on including a link to each endpoint's doc page in the corresponding function page anyways, so this might be okay.

from fredr.

Check out the updates to the branch:
https://github.com/DavisVaughan/fredr/tree/feature/function-parameters

devtools::install_github("DavisVaughan/fredr", ref = "feature/function-parameters")

Look at ?fredr_options, ?fredr_series and ?fredr_tags

from fredr.

Thanks! This is great stuff. Thanks for all the work.

• Endpoint consistent function names I agree. Since it will probably be the most commonly used function, what about exporting something like fredr_series_obs as an alias for fredr_series_observations for convenience? I don't want to reintroduce any confusion but it seems natural in my mind.

• Different defaults/limits for the same parameter across endpoints The more I think about it, the most straightforward approach may actually to be to document the optional parameters in each function so as to mimic the web documentation. The documentation for fredr_options(), which collects all parameters, can then be used to link to each function in which the parameter is used.
  For example,

  `limit`  An integer.  Used by `fredr_series_observations()`, `fredr_tags()`, ...
  

  where the functions listed after "Used by" are linked to their respective function documentation pages that specify how limit is used in that function (i.e. defaults and limits). Hence parameters are documented in a modular fashion each time a new function is implemented. Many options will simply need to be copy-pasted across functions.

  So I guess implicitly expressing a preference for clarity over expedience. It seems laborious but clean to me. We can do this for the core functions with the most parameters first. Does this seem like a worthwhile approach to you?

  Edit Kind of nit-picking here but if we adopt this approach, I'd like to see fredr_options() arguments organized alphabetically and the optional arguments in each function to mimic the order in the web documentation.

from fredr.

• fredr_series_obs is fine by me. long functions names don't really scare me as much as they used to, auto complete in most editors normally takes care of it. but i agree with you that this would be fine.

• I agree that we need to take a step backwards in light of the new information that alot of params are documented differently across endpoints. The more I think about it with this new info in mind, the more i'm unsure of using fredr_options(). It made sense when param docs were all consistent. Now that we have to document the params in each function's help file, what is the difference between making all relevant params explicit arguments to each function, vs using fredr_options()? A few benefits of going back to square 1 with the parameters as explicit arguments is that we get @inheritParams for less duplication (most of the time params are consistent so we still get benefits here), and autocomplete will show the users only the arguments that apply to that specific function without them having to check the help docs to see which apply. In fact, RStudio's autocomplete would even show the description of the explicit argument so it would tell them that limit goes from 0-100000 VS 0-1000 depending on the function they are using, without even having to consult the help docs.

A drawback of this approach is that each function's argument list becomes a bit messy.

fredr_series(series_id, options = fredr_options())

# VS

fredr_series(series_id, observation_start = NULL, observation_end = NULL, ...etc)

I'll leave this up to you, just let me know what you'd rather have.

(PS, I like nit-picking. It shows we care, and we end up with a better more polished result!!)

from fredr.

I propose we drop fredr_options() in favor of explicit function arguments in each function.

The argument list may be a bit messy (max number of args is about 12 it seems) this way but then the package will be closer to a simple set of R bindings to the API itself, which was my original vision for the package.

If you finalize this in fredr_series_observations() and send a PR, I will review and start working on the other implemented functions.

from fredr.

• Agreed about dropping fredr_options(). I think this is the cleanest way in the end and uses the native tools that rstudio and roxygen2 provides.

• I'll finalize it in fredr_series_observations() tonight, and PR it (I live on the East Coast of USA) so we can start working independently.

Since you gave me push access now I'll begin working on one of the new issues in its own branch here on this repo.

When I send the PR I'll explain how I do a few checks for argument validity and how I think the best way to do that is so that we can keep code duplication down.

from fredr.

This approach was implemented first for existing functions in #28

from fredr.