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

A few thoughts on the implementation #13

Closed
DavisVaughan opened this issue Jul 8, 2018 · 7 comments
Closed

A few thoughts on the implementation #13

DavisVaughan opened this issue Jul 8, 2018 · 7 comments
Assignees

Comments

@DavisVaughan
Copy link
Collaborator

DavisVaughan commented Jul 8, 2018

I would love to see this package on CRAN. I was just about to write an interface to FRED that does more than what we do in tidyquant (like accessing all the params of the API) when I discovered this. I think it has great potential!

If you don't mind, I'd like to suggest a few features that I personally would like. I'm hardcore biased towards tidyverse, so take it with a grain of salt.

  • Allow fredr_series to return multiple series in 1 call. Under the hood it could call map() to make multiple calls to the API, but it would be nice as a user facing feature to let us input a vector of series ids.

  • With regards to the above, it would then make sense to change from returning xts to returning a tibble. This could have a column, id that contained the series_id and you could stack series together into 1 tidy data frame. It would be up to the user to ensure that each series allowed for the specified frequency and other params, or those could be vectorized as well.

  • I'm not sure the fredr_key() function is working perfectly. I was not in an RStudio project, and tried to set it for the first time with fredr_key("my_key") and it errored out because I already had an .Renviron file. In riingo I let the user set the key temporarily with riingo_set_token() which sets an option() or they can set it permanently themselves by modifying their .Renviron file. This way I'm not directly messing with their .Renvion file.

  • The FRED API has a lot of useful endpoints. Would it make sense to expose all of them through something like fredr_series(), fredr_series_observations(), fredr_series_release(), ... and so on for series and for tags and everything else? It would work well with tab completion.

  • Lastly, I think the FRED API is relatively stable in terms of parameters that get passed through. To be easier on the user, maybe you could provide things such as series_id, observation_start, and all the other api params as function params for each of the above suggested endpoints. With roxygen2 and @inheritParams, it would really only be a matter of documenting them once, and sharing them across all functions that used them, so probably not too much work. This would be nice as it could keep the user from having to even go to the API docs for more info.

@sboysel
Copy link
Owner

sboysel commented Jul 8, 2018

@DavisVaughan: Thanks for using the package and for the thoughts!

  • CRAN: Yes, I had planned on submitting the package so I was taking the necessary steps to do so (CI, clear documentation, improving unit tests, etc.). I shelved the task due to lack of time but after working through some of these suggestions, I will revisit.
  • (Vectorizing fredr_series #16) Vectorizing fredr_series: I think this is a good idea. As of now, you could do something like
multiple_series_ids <- c("GNPCA", "GNP", "FEDFUNDS")
names(multiple_series_ids) <- multiple_series_ids
purrr::map_dfr(
  .x = multiple_series_ids,
  .f = function(x) fredr_series(series_id = x) %>%
    dplyr::rename(value = x),
  .id = "series_id"
)

I will prioritize as an enhancement.

@sboysel sboysel self-assigned this Jul 8, 2018
@DavisVaughan
Copy link
Collaborator Author

I can help too over the next week or so if you'd accept the PR!

@sboysel
Copy link
Owner

sboysel commented Jul 8, 2018

Absolutely. Thanks!

@DavisVaughan
Copy link
Collaborator Author

DavisVaughan commented Jul 19, 2018

I think things are looking pretty good. Want me to take one more look at the new vignettes? Anything else?

Update) I've added all my edits. Things are looking great!

@sboysel
Copy link
Owner

sboysel commented Jul 20, 2018

On CRAN submission: running into git2r authentication issue when running devtools::release()

Running Git checks for fredr
Checking uncommitted files... OK
Checking if synched with remote branch...
ERROR: Error in 'git2r_remote_fetch': error authenticating: 

I think it's related to recent changes in git2r. I can try downgrading my git2r so I can still use devtools::release().

I'll have to come back to this a bit later.

@sboysel
Copy link
Owner

sboysel commented Jul 21, 2018

I reran the git checks manually and submitted the package to CRAN. Will update on feedback.

A huge thanks for all the help, @DavisVaughan!

@DavisVaughan
Copy link
Collaborator Author

Happy to hear it and happy to help! Keep me updated and I'll promote on Twitter!

@sboysel sboysel closed this as completed Jul 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants