Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add messages for unparsable code caused by non-ASCII characters #642
Changes from 9 commits
af97fb6
deb934a
aeab0f2
c8d223d
2bd1378
8e88e07
fd2e251
f3920ab
018cb10
cfb4fc4
4eaa880
2815de4
a1e62da
4993584
29c797f
d2cefd4
0e8333d
e97825e
aeb5fa1
667ce95
c86c10e
d105baf
fffdad0
39b03ed
a4fbc33
4b36723
e4c15b7
f11247a
bf28dce
104ab32
be3813e
41785a4
8258fc9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
would highlight the
ó
indeviación
on line 2, missing the error that was actually caused by the±
on line 3.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.
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
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.
I added line numbers in bf28dce (currently lines are numbered even if there is only a single line of code).
Updated the suggestions to only give a replacement for one line in bf28dce.
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.