-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fetching Calendly Events #9
Conversation
@@ -1,37 +1,130 @@ | |||
# Calendly data extraction handlers | |||
|
|||
#' Get Calendly API user | |||
#' Handle Calendly GET requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now added a main function to handle GET requests. It is called by these other Calendly GET functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Both functions get_calendly_user()
and list_calendly_events()
work fairly well, if the arguments are fixed.
I should note that I haven't been able to test list_calendly_events()
for more than 100 events, since my Calendly account only has 4 events.
R/calendly.R
Outdated
#' authorize("calendly") | ||
#' get_calendly_user() | ||
#' } | ||
get_calendly_user <- function(api_key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_calendly_user <- function(api_key) { | |
get_calendly_user <- function(api_key = NULL) { |
The function needs a default of NULL for the example on line 56 to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added NULLs 👍
R/calendly.R
Outdated
if (is.null(api_key)) { | ||
# Get auth token | ||
token <- get_token(app_name = "calendly") | ||
} else { | ||
token <- api_key | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (is.null(api_key)) { | |
# Get auth token | |
token <- get_token(app_name = "calendly") | |
} else { | |
token <- api_key | |
} | |
if (is.null(api_key)) { | |
# Get auth token | |
token <- get_token(app_name = "calendly") | |
message("Using user-supplied token stored using authorize(\"calendly\")") | |
} else { | |
token <- api_key | |
} |
To improve the API design, I suggest adding a message telling the user that we will be using the user-supplied token from authorize("calendly")
. Kudos to Hadley's R Packages Workshop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent idea. Yes this whole package needs a lot more error handling and messaging improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added messages to warn users 👍
R/calendly.R
Outdated
|
||
#' Get Calendly API user | ||
#' @description This is a function to get the Calendly API user info | ||
#' @param api_key You can provide the API key directly or this function will attempt to grab an API key that was stored using the `authorize("calendly")` function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused with the argument api_key
as we are technically grabbing the personal access token. What about using token
as an argument instead?
#' @param api_key You can provide the API key directly or this function will attempt to grab an API key that was stored using the `authorize("calendly")` function | |
#' @param token You can provide the token directly or this function will attempt to grab the token that was stored using the `authorize("calendly")` function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been inconsistent in this. At some times it's an api key and sometimes it's a token. I've been realizing this as I've been writing but I've not yet fixed it. Because you've noticed I'll make sure to fix it now lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it more consistent that we always refer to it as a "token" instead of an "api_key"
R/calendly.R
Outdated
#' | ||
#' } | ||
#' | ||
list_calendly_events <- function(user, count = 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot to add api_key
as an argument:
list_calendly_events <- function(user, count = 100) { | |
list_calendly_events <- function(api_key, user, count = 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this missing argument.
Toward the end of getting this package pushed through, I'm not going to wait for @howardbaek to re-review to merge this since his first review didn't have major qualms and I addressed his minor qualms. |
Summary
In this PR, we fetch Calendly events listings!
It also involves some retrieving next pages so I built some pagination handlers. I might try to make this a more universal function for the other APIs that have pagination to handle. But one thing at a time.
Usage
The culmination of this PR is that you should be able to do something like this:
And the events object is a list that has events that have ever been scheduled for the user.