Skip to content

docs+dep: another pass and remove fetch_df #81

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Apr 24, 2023
Merged

docs+dep: another pass and remove fetch_df #81

merged 19 commits into from
Apr 24, 2023

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Apr 7, 2023 •
edited
Loading

Closes #84, closes #85

  • alias epidata_call to create_epidata_call
  • link to epidata_call for endpoint returns
  • update docs
  • \dontrun the private endpoint examples
  • removed fetch_df
  • print.epidata_call automatically displays the request url for the classic format
  • fix typo bug

@dshemetov
docs+dep: another pass and remove fetch_df
2cf0b13 
* alias epidata_call to create_epidata_call
* link to epidata_call for endpoint returns
* update docs
* \dontrun the private endpoint examples
* removed fetch_df
dshemetov and others added 5 commits April 6, 2023 18:01
@dshemetov
docs: consolidate fetch_* and other methods
7196ccc 
* fetch_* now in epidata_call docs
* request_rul and with_base_url there too
@lcbrooks
Correct typos, explain formats, use @rdname, fix print
612d3f8 
* Correct some typos: e.g., "and instance"
* Explain some unexplained formats: classic, json
* Use `@rdname` rather than `@describeIn` for the `?epidata_call` docs to try to
  get `create_epidata_call`, the thing that users are least likely to use, to not
  stick out so much over all the things that users would be using.
* Flesh out `?epidata_call` description further.
* Move `@rdname` to be the first tag, following advice from
  https://mirai-solutions.ch/techguides/advanced-usage-and-consistent-documentation.html,
  plus make `@description` explicit so that it can be above the Description:
  section without "Invalid file name" errors from roxygen2 thinking the
  description is part of the `@rdname`.
* Reorder some functions in the source file to make `?epidata_call` look better.
* Fix some missing roxygen tags preventing `print` and `as.data.frame` methods
  from being registered. Fixes #43. Use `print` in the `?epidata_call`
  documentation. Don't print "the" `request_url` because we don't know if the
  default arguments to `request_url` are what the user actually has in mind.
  Use `cli` to format it, as it's already a transitive dependency, looks nice,
  and is fairly easy to use.
* Add missing return description for `with_base_url`.
* Add example of using magrittr pipe to create an `epidata_call` + fetch from it
  without intermediate assignments.
@lcbrooks
Re-style_pkg() and make .editorconfig indent_size match
a78058c
@lcbrooks
Re-document() with styler changes
8902c48
@lcbrooks
Remove print(call) from examples
7b63552 
It no longer prints "the" request URL (ambiguous without knowing what the other
args to `request_url` should be), so it's not really "for debugging".  It's only
for pointing users to the fetching & `request_url` functions.
@brookslogan
Copy link
Contributor

Tried to do a simple tweak to the documentation here and it's bringing up check & implementation issues. We might just want to let the check() S3 notes slide for now, though I'm working to address them. (I wanted to use print(call) in the docs, addressed #43, found that print(call) didn't necessarily give the right request URL, check() finds that some print and as.data.frame methods have the wrong signatures, ...)

@dshemetov
Copy link
Contributor Author

dshemetov commented Apr 11, 2023 •
edited
Loading

Nice changes! cli is a nice touch! Question: what do you think of having print(call) provide the request_url by default (along with a pretty print of its params)? Or did you try this and run into a bug? It would be nice if the call pretty print was small enough to be viewable without scrolling (like a tibble preview). Maybe there's a pretty print package for lists that accomplishes that?

@dshemetov
Copy link
Contributor Author

dshemetov commented Apr 11, 2023 •
edited
Loading

Also: I really like that you show the piping example in the docs. I didn't want to add magrittr as a dependency, but that's really the workflow we want to model to our users. Maybe magrittr is light and common enough that we can just include it in Suggests and use pipes in all our examples?

@brookslogan
Copy link
Contributor

brookslogan commented Apr 11, 2023 •
edited
Loading

print(call) did provide "the" request_url by default before, but the problem is that the request URL changes based on what further options we provide; see the args to request_url. If you think if a better name for request-URL-with-default-args-for-request_url-but-other-arguments-baked-in, please add it!

Yes, I'd also prefer pipes in the examples. I did forget to Suggests: magrittr for how I went about this. Dependency-weight-wise, doesn't matter its weight because we already transitively depend on it via readr -> tibble -> magrittr. So maybe we could even just Imports: magrittr and re-export the pipe, so the examples don't even need library(magrittr)? In maybe 5 years we might consider the new R built-in pipe instead (|>), but I expect some of our potential user base to be stuck on old systems that don't have it available.

@dajmcdon
Copy link
Contributor

If you do usethis::use_pipe() it will set up the Imports / reexport version for you. That's my usual practice. Until built in pipe becomes common.

@dshemetov dshemetov mentioned this pull request Apr 11, 2023
Closed
@dshemetov
Copy link
Contributor Author

Sounds good, well if we already indirectly depend on it, then I'm happy to just import/export that pipe into the package!

Made a new issue for request_url in #84.

brookslogan
brookslogan approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow my review was stuck as pending. I think everything I noted + more has been addressed.

@dshemetov dshemetov merged commit 6dc0f71 into dev Apr 24, 2023
@dshemetov dshemetov deleted the ds/docs branch April 24, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brookslogan brookslogan brookslogan approved these changes

@dajmcdon dajmcdon Awaiting requested review from dajmcdon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Typo in flusurv column spec request_url handling of default args and other args
4 participants
@dshemetov @brookslogan @dajmcdon @lcbrooks