Closed charlie-gallagher closed 2 years ago
I have been wanting to add unit tests to the package but obviously the api requests were hindering that. Now that you have got the development in a much more modular focus, this makes a lot of sense. Thank you again for the hard work. Let me look over this and get back with you on where I think it should live while developing it.
@benrwoodard I've merged in changes from master, so that this has no conflicts any more.
Hi @benrwoodard, first, if you'd like to merge this into a different branch than 'master', feel free to redirect this.
Anyway, I was traveling the past few days so I took a shot at unit tests for the package using
testthat
. I didn't try to test everything, just those features that were easily testable without an internet connection.This led to a lot of refactoring, but no feature changes besides the changes I submitted in the other merge request. Fortunately, I didn't catch many existing bugs, but the revised code I think is much better and easier to use as we add new features.
I still consider this a work in progress, but I think it's a good step forward.
Summary
metric_container
no longer requires you supply a vector of metric IDs. You can generate a unique list of vector IDs with the utility functioncreate_metric_column_id
.top
argument into a utility function calledmake_explicit_top
. It doesn't replace zeros, but it obeys the rules for making implicit 'top' arguments into explicit values, and recycles 'top' while following the rules. I renamedtop_daterange_number
torecalculate_top_arg
to better describe what it does. It also now returns a named character vector (pairing dimensions and their respective 'top' values), which is good for debugging but otherwise has no effect on the results.global_filter
so that you can pass it arguments more intuitively. OnlysegmentId
anddateRange
are required, so you can focus on which values you want rather than where to put NAs like before. Thetype
argument is handled automatically as well, to reduce errors.request_format.R
,make_timeframe
,recalculate_top_arg
,make_expicit_top
, andformat_URL_parameters
.Requests for Comment
aw_get_reportsuites
had empty strings as defaults for two of its arguments. Why was that? I replaced it withNULL
to match the otherget_*
functions, but I thought that there might have been a reason.aw_segments_table
acceptsNA
values for segment IDs and then returns the metrics with no segment applied. This could be a handy feature -- maybe we should document that and return something better than we do now? I haven't experimented with this yet, though.aw_get_me
,aw_call_data
, andaw_call_api
have a lot of duplicated code among them. Maybe we should pull out some of the functionality into a more general function that we can test? This also applies to a certain extent to parts ofaw_workspace_report
. This has nothing to do with the current PR, except that it relates to unit tests.