search

NCEAS / rt

An interface to the Request Tracker API
https://nceas.github.io/rt/
Other
6 stars 4 forks source link

Fixed bug in search #13

Closed isteves closed 6 years ago

isteves commented 6 years ago

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 the inst folder (or somewhere else). I can then just test the data processing part of the process.

@amoeba

amoeba commented 6 years ago

LGTM, thanks!

I can't remember if we talked about changing the query param of rt_search to take a list. That might make it easier for the user to know what to type where. What do you think about that?

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.

amoeba commented 6 years ago

PS: If you wanna leave this as implemented, we can totally merge now and you can add anything else you want later.

isteves commented 6 years ago

I ended up going for the base R version since I haven't used purrr in the package yet.

amoeba commented 6 years ago

Right on. Not sure if you had this planned to add by I saw that plyr is still in the DESCRIPTION file.

isteves commented 6 years ago

Good catch! forgot to ctrl + s -___-

amoeba commented 6 years ago

Thanks!