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

check_result api change #11 #21

Merged
merged 34 commits into from
Jul 9, 2019
Merged

check_result api change #11 #21

merged 34 commits into from
Jul 9, 2019

Conversation

chendaniely
Copy link
Member

When this PR gets merged it Fixes #11.

@schloerke @garrettgman I believe I have all the working parts in there, but I can't seem to get the actual pass_if API working the way it was described in #11.

In the pass_if function signature, should the user_answer, grader_args = list(), learnr_args = list() all be passed as ...?

@rstudio rstudio deleted a comment from lintr-bot Jun 23, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 23, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 23, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 23, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 28, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 28, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 28, 2019
@rstudio rstudio deleted a comment from lintr-bot Jun 28, 2019
@chendaniely
Copy link
Member Author

chendaniely commented Jun 29, 2019

@schloerke I have a question of getting fail_if to work/pass tests:

The following test will fail

  expect_error(
    check_result(
        fail_if(~ .result == 5, "You were supposed to do this other thing!"),
        grader_args = list(solution_quo = quote(5)),
        learnr_args = list(envir_prep = new.env())
    )
  )

because I need one correct result to be passed (from check_result)

  if (!any(vapply(results, `[[`, logical(1), "correct"))) {
    stop("At least one correct result must be provided")
  }

The problem is, that the match exists. So should I be taking out that if statement? I'm not sure why it's there? Was it from the previous result api where one of the result values needed to be 'correct'?
The test will pass if I comment out the lines in check_result.

Full code is in the check_result.R and test_check_result_condi.R files in 851a5df

@rstudio rstudio deleted a comment from lintr-bot Jul 1, 2019
@rstudio rstudio deleted a comment from lintr-bot Jul 1, 2019
R/check_result.R Outdated Show resolved Hide resolved
R/check_result.R Outdated Show resolved Hide resolved
R/check_result.R Outdated Show resolved Hide resolved
R/pass_fail_condi.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@chendaniely
Copy link
Member Author

chendaniely commented Jul 8, 2019

@schloerke, If this review is good, can we merge this and work on #29 separately?

R/check_result.R Outdated Show resolved Hide resolved
R/pass_fail_condi.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments. both are minor.

LGTM once they're changed

schloerke
schloerke previously approved these changes Jul 9, 2019
@chendaniely
Copy link
Member Author

This PR also Fixes #29

@chendaniely chendaniely merged commit 5772c24 into master Jul 9, 2019
@chendaniely chendaniely deleted the check_result_api-#11 branch July 9, 2019 13:45
@chendaniely chendaniely changed the title [WIP] check_result api change #11 check_result api change #11 Jul 9, 2019
schloerke pushed a commit that referenced this pull request Jul 9, 2019
* add notes from meeting with baret

* pass_if formula (need to fix API)

* pass fail condi

* some tests for condi formula objects

* comment out sample code and add some comments

* tests for fomula condition

* remove old pass_fail_if code and tests

* lintr

* implement check_result pass_if with tests

* get fail_if test working

* check_result api for function and value

* break check_results result function api

* re-order function definitions (cosmetic)

* update evaluate_condi to return graded obj or NULL

* tests for evaluate_condi to expect graded obj instead of T/F

* implement check_result api changes

* remove extra line on top

* add ending newline

* documentation updates and condi -> condition)

* fix lintr whitespace

* roxygen doc changes after whitespace lint

* expect_error -> expect_wrong (namespace testthat::expect_error)

* pass in learnr_args not grader_args

* move graded to separate r script

* return the final_result$message, not message.

the pass_if test in `test_grade_learnr.R` was not passing along
the message from the `pass_if(message)` argument,
but rather, it was passing along one of the random .praise
values

* Rd change after graded was moved to own file

* barret review changes

* fix test grade learnr to pass

* remove last_value = quote() from test_check_result_condi

* fixes #29 test grade_learnr bug return graded obj after resu match

* evaluate_condi -> evaluate_condition

* allow for condi message to be used in full message

* add back break statement

* correct the message in a unit test
chendaniely added a commit that referenced this pull request Aug 9, 2019
chendaniely added a commit that referenced this pull request Aug 9, 2019
* comment out unused functions, move fxns to separate R files

* remove learnr dependency (broken)

* move and edit view/add/remove tutorial fxns

waiting for feature to be implement in rstudio before putting back in

also removes learnr depedency

* remove dependencies (clean up scripts and files)

removes call to pryr in grade_learnr and replace with rlang equlivilant

move unused functions to archive.
these are flagged for deletion.

move old tutorial files to archive,
will be added in when api is finalized,
and still deemed necessary.

* lintr!

* remove test and result function from archive.

result was removed in #21
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.

check_result API
3 participants