SDITools / adobeanalyticsr

R Client for Adobe Analytics API v2.0
Other
18 stars 9 forks source link

Store credentials in session environment #103

Closed charlie-gallagher closed 2 years ago

charlie-gallagher commented 2 years ago

This closes Issue #102. This makes it possible to auth with aw_auth() by passing client_id and client_secret manually. So, if you want, there is no longer a hard requirement for using environment variables.

Also, I updated the time-to-query calculation, which has worked well for me, but maybe you will have different results

Summary of changes:

Checklist

charlie-gallagher commented 2 years ago

@benrwoodard Also I noticed you fixed the character string dates handling, I merged all your changes in and tested before submitting this pull request, so R CMD check is still valid

charlie-gallagher commented 2 years ago

This may introduce breaking changes, since the arguments to some user-facing functions were changed. These were optional arguments, but still. Might add a lifecycle warning that says something like "Arguments client_id and client_secret are deprecated as they are no longer necessary

charlie-gallagher commented 2 years ago

A few more updates to fix bugs I found:

charlie-gallagher commented 2 years ago

This one is a tough one. I was pretty confident in the timing. The only area in question was to how many rows would be returned which will impact the number of breakdowns that will need to be called. How far off was the existing estimate?

So, I had to change the function that calculated the number of requests needed to complete the query (for use with the progress bar), and apparently this returned a different number in the estimate_requests function. I started getting ~22 secs as an estimate when I used to get ~32 secs or so. So I brought it back up, and it's been reliable

By the way, I can't see what changes you've requested -- I'm a little new to GitHub pull requests, but I should be able to see them shouldn't I?

charlie-gallagher commented 2 years ago

There's one more change I'm about to push, which changes the top_daterange_number function so it handles datetimes. Right now, it returns the wrong numbers if you have a 2-hour window, for example

benrwoodard commented 2 years ago

I would like to prevent a search that has a misspelled keyword and not throw an error to catch it as early as possible. Maybe it should be an '|' check so that if it is not empty or containing one of the keywords then throw the error. That way the function will prevent a user from going through the entire process only to find the search filter didn't work.

But this is a small error catch I guess in the grand scheme of things. The search function really should be expanded to be more flexible and easier on the user. I'm envisioning a separate step to build the search filters.

charlie-gallagher commented 2 years ago

I would like to prevent a search that has a misspelled keyword

I thought for a bit about how to catch search formatting errors, but since search strings can be complex and long, I couldn't find anything satisfying.

I agree that it should be easier, though. It might be out of scope, but we could include functions that build searches queries? Like an alternative to RSiteCatalysts selected argument would be something like:

make_selected <- function(items) {
  # paste together all the items so they make `(MATCH item1) OR (MATCH item2) OR...`
}