Skip to content

Conversation

@Matistjati
Copy link
Contributor

@Matistjati Matistjati commented Aug 20, 2025

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.

@Matistjati Matistjati marked this pull request as draft August 20, 2025 18:02
@Matistjati
Copy link
Contributor Author

I will split up the tex -> html/pdf into another PR. This PR is ready for review now

@Matistjati Matistjati marked this pull request as ready for review August 20, 2025 20:56
Copy link
Contributor

@gkreitz gkreitz left a 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
Copy link
Contributor

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
Copy link
Contributor

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? :)

Copy link
Contributor Author

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.

@gkreitz gkreitz merged commit 5e43d6a into Kattis:master Aug 21, 2025
4 of 5 checks passed
@Matistjati Matistjati deleted the multipass3 branch September 19, 2025 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants