datacamp / testwhat

Write Submission Correctness Tests for R exercises
https://datacamp.github.io/testwhat
GNU Affero General Public License v3.0
33 stars 24 forks source link

WIP Check arguments by position #228

Closed richierocks closed 5 years ago

richierocks commented 5 years ago

Allows you to pass a numbered argument to check_arg().

codecov[bot] commented 5 years ago

Codecov Report

Merging #228 into master will increase coverage by 0.02%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   92.09%   92.12%   +0.02%     
==========================================
  Files          47       47              
  Lines        2631     2640       +9     
==========================================
+ Hits         2423     2432       +9     
  Misses        208      208
Impacted Files Coverage Δ
R/check-function.R 100% <100%> (ø) :arrow_up:

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 3312b16...b034899. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #228 into master will increase coverage by 0.02%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   92.09%   92.12%   +0.02%     
==========================================
  Files          47       47              
  Lines        2631     2640       +9     
==========================================
+ Hits         2423     2432       +9     
  Misses        208      208
Impacted Files Coverage Δ
R/check-function.R 100% <100%> (ø) :arrow_up:

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 3312b16...b034899. Read the comment docs.

codecov-io commented 5 years ago

Codecov Report

Merging #228 into master will increase coverage by 0.02%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   92.09%   92.12%   +0.02%     
==========================================
  Files          47       47              
  Lines        2631     2640       +9     
==========================================
+ Hits         2423     2432       +9     
  Misses        208      208
Impacted Files Coverage Δ
R/check-function.R 100% <100%> (ø) :arrow_up:

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 3312b16...e6a2752. Read the comment docs.

richierocks commented 5 years ago

Reverting this to work in progress since it doesn't handle ... correctly. I think the problem is in the utility function find_function_calls().

richierocks commented 5 years ago

I modified get_args() so arguments in ... are now named ..1, ..2, ..3 (which is standard R naming). This means you can access them using check_arg("..1") etc.

For backwards compatibility check_arg("...") is the same as check_arg("..1").


There is one failing test for test_function().

https://github.com/datacamp/testwhat/blob/check_arg_by_position/tests/testthat/test-check-function.R#L551

All the tests for check_function() and check_arg() pass, so we might just be able to get rid of the legacy tests for test_function().

richierocks commented 5 years ago

closes https://github.com/datacamp/testwhat/issues/208

hermansje commented 5 years ago

Converting ... to ..1 might relax tests. This PR also destructures the ... argument, changing positional argument matching. My commit adds two basic tests for these cases and a solution for the first, that also fixes the test that failed.

The second case could be addressed by not destructuring ... and only allowing ..n syntax, which would require the implementation to be reworked.

hermansje commented 5 years ago

Before we merge, we should also document the ..n syntax.