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

Move error checking from render_exercise() to evaluate_exercise() #544

Merged
merged 25 commits into from Jul 9, 2021

Conversation

rossellhayes
Copy link
Contributor

@rossellhayes rossellhayes commented Jun 22, 2021

This is the first step in the process of moving error handling code out of render_exercise() and into evaluate_exercise()

Fixes #481

@rossellhayes rossellhayes marked this pull request as draft June 22, 2021 20:01
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

This is a great start and I really like that we'll collect all of the checking logic in one place in evaluate_exercise() rather than render_exercise().

The tests are currently written around the error checking logic being inside render_exercise() but now that we've moved that out we should

  • Update the error-checking tests so they call and reference evaluate_exercise()
  • Add tests the explicitly check that we get the correct expected errors from render_exercise()

R/exercise.R Outdated Show resolved Hide resolved
tests/testthat/test-exercise.R Outdated Show resolved Hide resolved
@gadenbuie
Copy link
Member

gadenbuie commented Jun 23, 2021

For next steps, we can update the temp directory logic so that we stay in the temp directory while evaluating the checker functions as well. So rather than with_dir() we would use setwd() and on.exit() to coordinate that step. You might also want to move the code checking logic up a bit; there's no need to do anything with the exercise temp directory until after we've performed the code checking.

R/exercise.R Outdated Show resolved Hide resolved
@rossellhayes rossellhayes marked this pull request as ready for review June 27, 2021 05:14
We may want to use it in other places
Copy link
Member

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@gadenbuie gadenbuie merged commit cb9ba29 into rstudio:master Jul 9, 2021
@rossellhayes rossellhayes deleted the evaluate-conditions branch July 20, 2021 21:41
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.

render_exercise() should use rlang::return_from() when an error is found
2 participants