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

Separate API calls to internal functions. #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ClaytonJY
Copy link
Contributor

All functions except drop_auth, drop_download, and drop_upload now use an internal API wrapper (named like api_*) to interact with the Dropbox API.

I put all the functions in the relevant file, at the end. Standardized usage with a new post_api utility function.

Fixes #109.

@karthik
Copy link
Owner

karthik commented Oct 19, 2017

Nice! I was going to start on this over the weekend/early next week but I'll review this Monday or Tuesday. 🎈

@ClaytonJY
Copy link
Contributor Author

ClaytonJY commented Oct 19, 2017

@karthik cool, no rush. Some additional notes:

  • no end-user apparent changes here, just refactoring.
  • added tibble as an import so I could use lst; doesn't actually add a dependency since we import dplyr
  • I picked api_* for the common name format of the API functions; pretty arbitrary, but they're internal, I liked keeping them separate from the drop_*'s
  • post_api is kinda weird; more than open to new names. Arg order is different, but I didn't want to put the token after the ... so I didn't have to sweat naming that arg every time
  • in the long-run, we should probably have strong assertions in the api_ functions; skipped for now both to minimize effort and keep this PR manageable. Save that for the new package.
  • probably kills test coverage; I think most of this code was already hit in tests, but now it takes up fewer lines. PR is only net-positive on lines because of barebones function comments on new funcs.
  • didn't put any real thought into doing anything with drop_auth/drop_upload/drop_download here; much trickier. Could probably share some code between upload/download, not sure about auth. Maybe post_api should have another sub-function that's even less opinionated, and could be used to build auth/upload/download requests too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants