Streamlined setting environment variables into setup_ckanr, superseding set_test_env

[florianm]

Following up on discussion of #31 - first cut - @sckott @wush978 what do you think?

[florianm]

Ok, this is starting to become a major update - thanks to @wush978 things are now clearer and more consistent but in the course of this refactoring I've had to touch ckanr in a lot of places.

• set_ckanr_url() and set_api_key() are gone, superseded by ckanr_settings().

• functions default to url=get_default_url() and key=get_default_key().

• ckan_POST now expects a key for authenticated requests - no more defaulting to api_key() - authenticated functions now use ckan_POST and hand over a working key.

• tests consistently use the configured test server - removes and centralises some brittle assumptions from tests

• some test settings, getters and test helpers added for groups and organizations.

• ISIS mode for tests - blow up or skip - set through test_behaviour.

• major formatting spring clean (line length, white spaces in function arguments).

• put my email address to the authors field if that's ok

  @sckott sorry for the build error - you'll need a few new environment variables (see setup_ckanr).

# Scott Chamberlain @ data-demo
setup_ckanr(test_url="http://data-demo.dpaw.wa.gov.au/",
            test_key="scotts-test-key",
            test_did="ae6fdf6f-1435-444b-b319-198cc84e4594",
            test_rid="0155c395-842d-4b66-a72c-c6e688b260c3",
            test_gid="templates-and-guidelines",
            test_oid="sandbox",
            test_behaviour="FAIL")
devtools::test()

Looking forward to constructive feedback!

[sckott]

@florianm can you undo these changes to spacing around = signs? I think many agree that code is more readable with spaces in appropriate places. In all places you changed

[florianm]

Yelp... OK I'l bring them back! I went with the Google R style guide which finds spaces being optional for function parameters. Agree with Hadley's "always spaces around infix operators".

[sckott]

In the future, smaller pull requests that are focused around a single theme, would be easier to manage.

I made a comment about white spaces and parameter newlines. I'd like it if you could undo those changes. We generally follow http://adv-r.had.co.nz/Style.html

Thoughts @wush978

[wush978]

I check this PR on windows but it fails:

==> devtools::test()

Loading ckanr
Testing ckanr
changes : 12...
ckanr_settings : .................
ds_create_dataset : ...34
ds_search : 56
ds_search_sql : 78
group_list : 9abc
changes : defg
license_list : h
organization_list : Error in function (type, msg, asError = TRUE)  : <url> malformed
Calls: <Anonymous> ... perform -> <Anonymous> -> .Call -> <Anonymous> -> fun

A minor concern:

I do not suggest to use:

 url <- get_test_url()
 key <- get_test_key()
 rid <- get_test_rid()
resource_update(rid, path=path, url=url, key=key)

Because this kind of test does not coverage the real use case:

setup_ckanr(url="http://data.techno-science.ca/", key="my-ckan-api-key")
resource_update(rid, path=path)

[sckott]

@florianm I would suggest making this function to set settings the same name format as the function to get settings: So ckanr_setup and ckanr_settings, that okay?

[florianm]

That would be even more consistent, will do!

[sckott]

great 👍

[sckott]

@wush978 you have to set a few env vars - see #37 (comment)

After setting those, tests did pass for me locally

[sckott]

@florianm regarding @wush978 point about get_*() functions, is the purpose of those functions to be able to use in the test suite so that we, or other users, can pass in the url/key/etc. as a parameter instead of functions reading env vars after using setup_ckanr()?

[florianm]

Hi all, Thanks for the feedback, I'll get to incorporate all of your requests hopefully on Monday! They all make sense. With the latest status quo, do we need a separate test url and key, or should tests set and implicitly/explicitly use the default url and key? Cheers and thanks for the review, Florian

[sckott]

@florianm wrt putting each param on a separate line - I imagine you did so so it's easier to run default params to work on the function? I totally understand that. That's why I use this https://github.com/sckott/sacbox/blob/master/R/load_defaults.r I just load that on every R session, so that I can load default params for a function I'm working on, and don't need to run one at a time.

[sckott]

With the latest status quo, do we need a separate test url and key, or should tests set and implicitly/explicitly use the default url and key?

I guess CKAN is a little different from other things in that a user could potentially be a developer/data provider and want to test against different servers with different auth settings. So maybe it makes sense to set different settings for test server

[florianm]

Thanks for the feedback, good points!

• re PR size - apart from formatting (which I'll undo), as this PR changes some core workflows (and @wush978 rightly wouldn't let me get away with half-way solutions) I found no other way than going all in.
• re param line breaks - to my Python-based taste, readability outweighs brevity - lists of parameters passed to functions longer than 80 chars looks better when line-fed individually. It's easier for caffeine-deprived developers like me to spot omissions while coding (especially when adding/removing parameters), and to end-users those line breaks will be hidden. But that's only my personal taste and as you suggested, I'll condense the parameters again.
• re parameter=default spacing - see above, went with Google style, will convert to Church of Hadley. Law, order and aesthetically pleasing spacing is only one grep/xargs/sed away.
• re default vs test url and key - with my initial cut, having separate default and test settings made sense to me. But @wush978 has a valid point in wanting tests to use the default url and key implicitly. If we can assume that testers will run ckanr_setup before running the tests anyways, and not use the same ckanr instance against a production server while the tests are running (right?), then we won't need separate url and key, and we can simplify the parameter names. So end-users would still run

ckanr_setup(url="sdfsdf", key="sdfsf")

while testers would run (if we simplify the parameter names):

ckanr_setup(url="test ckan", key="test key", did="test ds id", rid="test res id", gid="test group name",
oid="test org id", test_behaviour="FAIL")

# testing whether function defaults to ckanr_settings correctly:
x <- do_something() # defaulting to get_{url, key, did, rid, gid, oid}()

# testing with explicit parameters
y <- do_something(url=get_url(), key=get_key(), did=get_did(), rid=get_rid(), gid=get_gid(), oid=get_oid())

• re load_defaults.R - nice. I will totally steal this - but setting defaults like this was neither within my skill set nor my reasoning :-)

So, last question to you @sckott and @wush978 :

Should I keep DEFAULT and TEST url and key separate, or merge and simplify? If we merge, what parameter names would be the best (should I drop the "test_")? Is there a shorter name for "test_behaviour"?

[florianm]

Formatting clean-up is done, tests pass with Scott's settings on data-demo.dpaw.wa.gov.au. Pending my actions on whether to keep or merge default and test url/key, this PR is nearing completion.

[sckott]

Thanks @florianm

Should I keep DEFAULT and TEST url and key separate, or merge and simplify? If we merge, what parameter names would be the best (should I drop the "test_")? Is there a shorter name for "test_behaviour"?

@wush978 do you have an opinion on this?

[wush978]

Sorry that I cannot closely check these codes. I will check them on the weekend.

[sckott]

@florianm I think this is good. Mind if I merge now?

[florianm]

All good from my side, and if you and Wush feel merging default and test url/key would be appropriate I can still do so in a new PR.

[sckott]

Yeah, I'd like to get this merged - and we can make additional changes after merging - if needed

[wush978]

Dear @florianm ,

I think dropping test_ is ok. However, it is not an urgent issue, so maybe we could change it in the future.

[florianm]

Thanks @wush978 and @sckott - let me know if you wish to merge and I'll submit another PR.

[sckott]

@florianm merge what? this is already merged

[florianm]

Sorry - I meant url and key default and test settings (along these lines)!

[sckott]

@florianm yes, plz do.

regarding style - in ropensci pkgs we try to maintain a similar style across pkgs for consistency within ropensci. totally fine with other styles, but we just want to maintain consistency within our suite as much as possible