-
Notifications
You must be signed in to change notification settings - Fork 78
Multipass support #350
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
Multipass support #350
Conversation
|
I will split up the tex -> html/pdf into another PR. This PR is ready for review now |
gkreitz
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.
Looks good. Added two nit-picky comments, but they are so small I'll just merge this. We can fix those in a future PR if we want/care.
| if interaction.startswith('---'): | ||
| passes.append(curr_pass) | ||
| curr_pass = [] | ||
| continue |
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 continue could probably be an else (and I think that'd be preferable)
| return SubmissionResult('JE', reason=f'failed to parse validator score: {e}') | ||
| else: | ||
| return SubmissionResult('JE', reason='problem has custom scoring but validator did not produce "score.txt"') | ||
| # If we're running multipass, we do not need to output a score after every pass |
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.
Do you actually need this check? Multi-pass can only happen in the new format, where custom score is never mandatory. So I think you could have left this else-clause as-is?
(If we do in fact need it, can't we just check both that the problem is multipass and that nextpass.in exists, making the check correct and avoiding a scary comment about accepting a small risk? :)
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.
Yes, that's true. I somehow missed that we actually have a reference to the metadata via self.problem in this class. Will add both of these in the multipass sample PR.
I dislike the inline functions on line 415 and 421, but ruff demands it...
I do not try to prevent the submission from passing information out of bound. In my opinion, doing so would just be cosmetic, as it's impossible to do it fully without building a sandbox.
In a follow-up PR, I will fix multipass sample rendering when the source is tex. I have fixed it for md -> html in this PR.