-
Notifications
You must be signed in to change notification settings - Fork 242
Fix and improve knitr chunk option handling #668
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
Conversation
rossellhayes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, I mostly just had a few questions!
| if (is_exercise_engine(exercise, "sql")) { | ||
| rmd_src_user <- c( | ||
| rmd_src_user, | ||
| "", | ||
| '```{r eval=exists("___sql_result")}', | ||
| 'get("___sql_result")', | ||
| "```" | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we add support for more engine, will this become a selector that calls from a list of helper functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I think that it'll be a little more clear as we start testing more engines. In this case we're appending a chunk, but we might also need to prepend -- or maybe even use an entirely different template. So we'll see.
and master -> main
Fixes the underlying problem reported in #667 and improves support for
sqlexercises.Fixes #667
Fixing knitr chunk option handling
The bug in #667 is that we were not properly creating the
knitr_optionsobject that is used by the base format that renders the exercise code. We weren't moving the exercise options inopts_chunkas needed, now by putting these in the right place they are used as expected.As part of fixing the bug above, we are now delegating more to the
prepare_exercise()helper. The goal is that any preparation required to special-case different types of exercises should happen here. For example, there's some small re-arranging of exercise options that is needed to smoothly support SQL chunks; this now happens insideprepare_exercise().After passing through
prepare_exercise(), the exercise object gains an$opts_chunkitem which contains the prepared knitr options for the exercise rendering. Previously we were pulling fromexercise$options, but those might be slightly different. Note that we don't touch those options so that we don't lose the original exercise options — someexercise.*options are stored there but aren't needed for rendering and are removed from$opts_chunk.As part of this work, I also refactored
merge_options(). It still follows the same general structure, but I've renamed variables to be a little more explicit.Improved SQL Exercise Support
This PR also improves support for SQL exercises (and in general non-R language exercises). SQL exercises have been a good test case since they involve a number of pieces that will likely arise in other exercise situations:
Test for engine (language) types. First, we can now use
is_exercise_engine(exercise, engine)to test if an exercise uses a particular engine. The engine type applies to the user code and the solution code; other supporting chunks may be in R (or other languages).Pre-process the exercise. As described above, we may need to re-arrange the exercise data to prepare for rendering. This includes setting global knitr chunk options, adjusting specific chunk options on the exercise chunk, adding setup code, etc. This action should be handled in
prepare_exercise(), delegating to prep functions for specific engines, e.g.prepare_exercise_if_sql().In
sqlexercises, we add or force theoutput.varchunk option to the exercise chunk to something predictable. When rendered, the results of the SQL query will be stored in a predictable location, allowing us to provide the result to the checking functions as thelast_value.Adjust the
exercise.Rmdtemplate. The knitr SQL engine either prints the result of a SQL query or stores the result in the variable named byoutput.var. Since we want to both see the printed output and capture the result, we need to add additional code to theexercise.Rmdtemplate.I added
exercise_code_chunks_user_rmd(), which now prepares the contents ofexercise.Rmd. In general, this involves reading from the internal exercise template and appending the exercise chunk with the user's submitted code. If additional processing is required, we can detect the exercise engine and add additional lines to this file, as in the case of the SQL engine.Post-process exercise results. After rendering, we may need to post-process the results to put them in the expected places. In the SQL case, we do two things:
envir_prepwith the name from theconnectionchunk option. This lets grading code find the connection — in gradethis the connection object will automatically be present insidegrade_this().output.varchunk option on the exercise, we ensure that the results are available under that object name inenvir_results. If not, we remove our temporary variable___sql_resultssince it's not needed. (If this sounds convoluted, trust me, it's easier than dealing with the many states of knitr chunk options.)Example
I've added
tests/manual/sql.Rmdas an example of a SQL exercise. A minimum version of a SQL exercise looks like this:Note that
connectionis required and it must be a string:connection = "db_con". Using the bare symbol will result in an error.You don't need to include the
output.varoption on the exercise chunk, but if you do — e.g.output.var = "my_result"— it will be available insideenvir_result. (In gradethis, you'd be able to get it via.envir_result$my_result.)Technical Notes
This PR adds a few packages to the dependencies of learnr. I tried to find ways of minimizing those additions, but I don't see a way around them:
We needTurns out thatmethods::is()to be able to check if an object is aDBIConnection, since those are S4 methods. Since this is a base package, it won't count against us.inherits()works fine in this situation.DBIandRSQLitein Suggests so that we can test that sql exercise evaluations works as expected. I addedskip_if_not_installed()for both packages to those tests, but unfortunately they need to be in Suggests or R CMD Check will issue a warning. (We could get extra crafty but I'm not sure its worth the effort.)Also, I had to spend a lot of time in
exercise.Rso I did a little re-organizing to help maintain my sanity. (f971aca(#668),b20ca76(#668),f347f80(#668))I made
knitr_engine_caption()a static export (that we immediately re-import) because it's also used by gradethis.Speaking of gradethis, rstudio/gradethis#290 is the sibling PR that improves grading for non-R language exercises (and is required for
tests/manual/sql.Rmd).