Fixed bug in search #13
Conversation
|
LGTM, thanks! I can't remember if we talked about changing the For your issue of wanting to test your processing code with a formal unit test, your intuition is right on. The way people like to do this is to make what's called a mock. It's basically what you said: save a copy of some response to some function call that usually is slow/can fail/etc and test that. This makes unit test unit tests and not integration tests! I haven't done this directly with testthat, but there appears to be support: https://github.com/r-lib/testthat/blob/master/R/mock.R. Also see https://www.mango-solutions.com/blog/testing-without-the-internet-using-mock-functions. |
R/rt_search.R
Outdated
| url <- paste0(base_api, "/search/ticket?query=", query) | ||
| #based on httr::modify_url() | ||
| #possible TODO - turn this into its own function that can be used internally in the package | ||
| l <- plyr::compact(list(query = query, |
There was a problem hiding this comment.
Cool, hadn't seen plyr::compact. Do you think it's possible to do this without adding another dependency? Can tidyr do this so we don't need another dep?
There was a problem hiding this comment.
I can't seem to find it in dplyr/tidyr version of it, though I've noticed people sometimes include the code of other functions in a package to avoid exporting it (I've seen this in particular for %>%). It seems like this could lead to some naming conflicts if both are loaded, but perhaps that's okay in this case?
I asked Twitter and will maybe get an answer there...
There was a problem hiding this comment.
It seems like this could lead to some naming conflicts if both are loaded, but perhaps that's okay in this case?
I'm not totally sure if you would so long as you don't export the binding.
The code for plyr::compact is just: function(l) Filter(Negate(is.null), l)
so it'd (1) easy to re-write in purrr or just copy the body of the function in and add a comment like # In-lined from https://github.com/hadley/plyr/blob/fe192412748d1ccb6d524caeb5bcf2b5c474fd46/R/utils.r#L42
There was a problem hiding this comment.
Ok, so apparently purrr::compact() exists, so I'll just use that (it's listed with keep in the documentation)
|
PS: If you wanna leave this as implemented, we can totally merge now and you can add anything else you want later. |
|
I ended up going for the base R version since I haven't used purrr in the package yet. |
|
Right on. Not sure if you had this planned to add by I saw that |
|
Good catch! forgot to ctrl + s -___- |
|
Thanks! |
I reworked the search processing with some better regex-ing, and now
rt_search("Queue='arcticdata'")works without failing! (at least for me...) I really would like to write some tests, but I'm not sure if we can avoid pinging RT. If you've had any further thoughts on this, let me know.One possibility is to have sample
content(request)results stored in theinstfolder (or somewhere else). I can then just test the data processing part of the process.@amoeba