Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply assertions to VCRConfig's initial arguments #156

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

aaronwolen
Copy link
Member

This is mostly an academic change with zero functional impact for users but I think it's the "correct" approach for adding assertions to R6 classes, so it might be useful as a reference if nothing else.

In the current implementation assertions are only applied when updating the fields of an existing object, so it's possible to initialize an object with an invalid value:

config <- vcr:::VCRConfig$new(record = "once")
config$record <- 1
#> Error in check_record_mode(value): is.character(x) is not TRUE

vcr:::VCRConfig$new(record = 1)
#> <vcr configuration>
#>   Cassette Dir: .
#>   Record: 1
#>   URI Parser: crul::url_parse
#>   Match Requests on: method, uri
#>   Preserve Bytes?: FALSE
#>   Logging?: FALSE
#>   ignored hosts: 
#>   ignore localhost?: FALSE
#>   Write disk path:

With this new change, initial arguments are passed to the active binding rather than being assigned directly to their private field, so assertions are applied in both cases:

config <- vcr:::VCRConfig$new(record = "once")
config$record <- 1
#> Error in check_record_mode(value): is.character(x) is not TRUE

vcr:::VCRConfig$new(record = 1)
#> Error in check_record_mode(value): is.character(x) is not TRUE

Created on 2020-02-19 by the reprex package (v0.3.0)

You have way more experience with R6 so I'm curious to see what you think and whether I'm missing something.

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #156 into master will increase coverage by 2.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   75.51%   77.63%   +2.11%     
==========================================
  Files          36       36              
  Lines        1511     1511              
==========================================
+ Hits         1141     1173      +32     
+ Misses        370      338      -32
Impacted Files Coverage Δ
R/configuration.R 64.91% <100%> (+28.07%) ⬆️

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 ffd1549...8e79bc9. Read the comment docs.

@sckott sckott added this to the v0.4.6 milestone Feb 19, 2020
@sckott
Copy link
Collaborator

sckott commented Feb 19, 2020

good catch on making sure user inputs are checked on initialize as well.

using active binding on init seems smart to me.

ran tests and such and don't see any issues.

@sckott sckott merged commit 0d29061 into ropensci:master Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants