Overhaul vcr_configure() to avoid overwriting existing config parameters

[aaronwolen]

This addresses #136 and changes the behavior of vcr_configure() so that only parameters passed directly as arguments are updated, which avoids overwriting the existing configuration.

Details:

• adds a new test to verify previous settings are not overwritten
• configuration defaults are now defined within the VCRConfig class
• vcr_default_config_vars is no longer necessary and has been removed
• configuration assertions are implemented as active bindings inVCRConfig
• assertions for record modes and request matchers are now implemented in check_record_mode() and check_request_matchers(), respectively, so they can be used by cassettes and the config
• adds more tests for vcr config parameters

New VCRConfig methods:

• reset()
• fields()
• as_list()

Happy to discuss if you have other ideas about how this should be implemented.

[sckott]

thanks, having a look

[sckott]

looks pretty good

• There's some failing checks https://travis-ci.org/ropensci/vcr/jobs/642204658 if those could be addressed
• The parameters in the configuration.R file are now not used, causing errors in cran check. I guess make a list of those params instead?
• Since vcr_configure is now vcr_configure(...) all parameters must be named. can you change egs in that file to all be named. And add a test in test-configuration.R addressing this

[aaronwolen]

There's some failing checks https://travis-ci.org/ropensci/vcr/jobs/642204658 if those could be addressed

My fault. I thought the failures were caused by a pre-existing issue (e.g., 640999928) but that wasn't the case. Looks like everything's passing now that I've fixed the documentation problems you pointed out.

The parameters in the configuration.R file are now not used, causing errors in cran check. I guess make a list of those params instead?

Ah, I overlooked how this would affect documentation. I went ahead and converted the param docs to a list as suggested. One nice benefit of this approach is we have more control over formatting. I took advantage of this and split-up the args into different sections, attempting to make the long list more easily scannable.

But maybe this isn't the best approach. I initially opted for ... simply to avoid having to list all of the configuration arguments in yet another location but there are disadvantages:

• we're losing potentially valuable documentation checks, and
• users can't use autocomplete to list all of the possible args

Perhaps I should restore vcr_configure()'s named arguments and document the old fashion way?

[sckott]

thanks for the fixes.

wrt docs: true, we don't get the docs check. maybe we could include checking the list of params in the docs against the allowed set?

wrt number of parameters: I get the strong impression that having a lot of parameters in function/method is not ideal. some blogs/etc. say you can't have a hard and fast rule, but seems like folks lean away from a huge number of parameters (as we used to have) and move towards what you have done with ... or equivalent in whatever language they are working in - and then the internals of the function/method check if the params passed in are in some allowed set. so anyway, i think this is the better approach but yes, we do lose the docs check on the param definitions

[aaronwolen]

Cool, then I'll leave it as is.

maybe we could include checking the list of params in the docs against the allowed set?

That'd be great. Are you thinking we should parse vcr_configure.Rd and extract the list of params?

[sckott]

parse vcr_configure.Rd ...

yeah, unless there's something better?

[aaronwolen]

Unfortunately I'm not aware of a better approach either.

I added a helper function to extract the documented args from the Rd file and a test to compare the output against the function arguments.

It's... not pretty but works. Let me know if you have suggestions for simplifying.

[codecov-io]

Codecov Report

Merging #141 into master will decrease coverage by 4.11%.
The diff coverage is 28.22%.

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
- Coverage   78.77%   74.66%   -4.12%     
==========================================
  Files          36       36              
  Lines        1409     1484      +75     
==========================================
- Hits         1110     1108       -2     
- Misses        299      376      +77

Impacted Files	Coverage Δ
R/cassettes.R	80% <ø> (ø)	⬆️
R/use_cassette.R	100% <ø> (ø)	⬆️
R/logger.R	69.56% <ø> (ø)	⬆️
R/cassette_class.R	78.57% <100%> (-0.83%)	⬇️
R/zzz.R	88.67% <100%> (+5.82%)	⬆️
R/check_cassette_names.R	90.9% <100%> (ø)	⬆️
R/configuration.R	16.66% <13.59%> (-41.48%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 345fb19...5e00ece. Read the comment docs.

[sckott]

thanks, having a look