-
Notifications
You must be signed in to change notification settings - Fork 240
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
Add messages for unparsable code caused by non-ASCII characters #642
Conversation
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 is a great start! I just have a few suggestions for a final round of refactoring and polish.
Co-authored-by: Garrick Aden-Buie <garrick@adenbuie.com>
Co-authored-by: Garrick Aden-Buie <garrick@adenbuie.com>
…able_i18n_span()`
R/exercise.R
Outdated
) | ||
} | ||
|
||
exercise_highlight_unparsable_unicode <- function(code, pattern) { |
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.
How does exercise_highlight_unparsable_unicode()
handle matches on multiple lines? I think we should only present one line at a time.
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.
Right now it shows all lines. I considered both, but leaned towards including all lines because:
- Exercise input is generally only a few lines maximum.
- If we show only one line, we might highlight a false positive, and the students won't get to see the later line that caused an error, e.g.
media <- mean(x)
deviación <- sd(x)
media ± 1.96 * deviación
would highlight the ó
in deviación
on line 2, missing the error that was actually caused by the ±
on line 3.
- It's more helpful to include all lines in the suggested replacement, since we'd expect e.g. all the quotes in the response to be curly instead of straight. It feels more symmetrical to include all lines in both the highlighted code and the suggested replacement.
That was my thinking, but I'm happy to change if you think it would be better to include just one line
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.
My thinking was that exercises are frequently long enough that we should try not to put the whole exercise in the lint and suggestion. I've seen many examples where an exercise is a dozen or more lines long. If we end up repeating the input code twice, that starts to get out of hand.
I think we should see if we can use the parse error message to isolate the problematic line and produce more targeted feedback.
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.
Updated in f11247a, the message now looks like this:
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! Two thoughts, not necessarily blockers. We could maybe add in line numbers for the first snippet.
2: ”test”,
We might also want to provide a fix only for the line we show in the first snippet. Even in the screen shot this seems kind of long. We could change
- You can try replacing your input with this code:
+ You can try fixing the code on that line with the following. There may be other places that need to be fixed, too.
This will help ensure that we don't incorrectly highlight valid non-ASCII characters. If there are extra problematic characters, students either get additional practice fixing the problem or they will get another targeted piece of feedback if they miss a character on another line.
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.
Should we include line numbers even if the student's code is only one line, or only for mutliline input?
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.
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.
Yeah, if only to keep the text message consistent. If we can keep the text message the same for 1-line and n-line submissions, then it'd be fine to drop the line number.
Co-authored-by: Garrick Aden-Buie <garrick@adenbuie.com>
…)` and move much of the logic of `exercise_check_unparsable_unicode()` into it
Clarify from the function signature the purpose of this argument
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.
Excellent! This will be a very helpful feature for anyone who gets caught off guard by copy-paste shenanigans!
This PR adds new messages for cases where students submit unparsable code with non-ASCII characters. This should help students who have pasted from a source that applies automatic Unicode formatting. This code would otherwise be very difficult to debug, as the Unicode characters may be difficult to distinguish from ASCII counterparts.
In cases where students have copied code with Unicode quotation marks or dashes, the prompt will automatically suggest a replacement with straight quotations marks or the ASCII hyphen-minus. In other cases, the message will simply suggest students retype the offending character manually.
Note that the message will not display if students write parsable code that includes non-ASCII characters, such as in character strings or (otherwise valid) variable names.
Examples
Unicode ("curly") quotation marks
Unicode dash
Other special characters
Parsable non-ASCII characters
PR task list:
devtools::document()
Closes #639.