-
Notifications
You must be signed in to change notification settings - Fork 2
Add support for workspaces and serverFilesQuestion #76
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
Credit: Jonatan Schroeder for the original question that was converted to markdown here
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.
Left some comments to help understand my thought process here, since I did a bit more than just base workspaces looking at it now.
I can move some changes to a different PR (or just delete them entirely if they're unwanted)
|
||
if os_errors: | ||
error_msg = "\n ".join(os_errors) | ||
raise FileNotFoundError(f"Error(s) copying specified files:\n {error_msg}") |
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.
The idea behind the try-except system here is to show all file copy errors to the best of our ability at once, instead of having it show up one file at a time.
The output of this should look something as follows, as an example:
FileNotFoundError: Error(s) copying specified files:
[Errno 2] No such file or directory: 'test0'
[Errno 2] No such file or directory: 'test1'
[Errno 2] No such file or directory: 'test2'
[Errno 2] No such file or directory: 'test3'
[Errno 2] No such file or directory: 'test4'
[Errno 2] No such file or directory: 'test5'
[Errno 2] No such file or directory: 'test6'
[Errno 2] No such file or directory: 'test7'
[Errno 2] No such file or directory: 'test8'
[Errno 2] No such file or directory: 'test9'
tests/test_question_templates/question_inputs/q16_workspaces/q16_workspaces.md
Show resolved
Hide resolved
tests/test_question_templates/question_inputs/q16_workspaces/q16_workspaces.md
Show resolved
Hide resolved
tests/test_question_templates/question_inputs/q16_workspaces/q16_workspaces.md
Show resolved
Hide resolved
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.
The changes look good to me, and the processed question works as expected.
It would be more ideal for the test to be in its own question, but at the moment I discern a simple test case that would test this elegantly
I think I looked at this a while ago, but forgot to merge it in... hopefully nothing has changed since then. |
While adding the copy files code for workspaces/serverFiles, I updated the code such that it should now catch a wider set of missing files at once to help fix questions faster, instead of one file at a time.
It would be more ideal for the test for
serverFilesQuestion
to be in its own case I admit, but at the moment I cannot discern a simple test case that would test this elegantlyResolves #75