Kattis / problemtools

Tools to manage problem packages using the Kattis problem package format.
MIT License
101 stars 70 forks source link

Cleanup `problem2{pdf,html}` and use f-strings in `verifyproblem` #237

Closed JoelNiemela closed 9 months ago

JoelNiemela commented 9 months ago

For problem2{pdf,html}:

For verifyproblem:

Tagl commented 9 months ago

There is a bare except with no specified exception type in template.py, maybe clean that up too while you're at it.

JoelNiemela commented 9 months ago

There is a bare except with no specified exception type in template.py, maybe clean that up too while you're at it.

I considered it, but I can't find any documentation that lists which exceptions can be thrown by "file object".write.

simonlindholm commented 9 months ago

except Exception is better than except: except if the catch block does cleanup and then rethrows, since except: catches things like thread/program cancellation as well as error exceptions.

In this case it looks like line % data is the part that is expected to throw, rather than .write(), and that probably only throws a single kind of exception.

Tagl commented 9 months ago

IOError and OSError are two at least, maybe there are more. My idea was simply to have it Exception though instead of catching everything.

On Tue, Sep 26, 2023, 21:01 Simon Lindholm @.***> wrote:

except Exception is better than except: except if the catch block does cleanup and then rethrows, since except: catches things like thread/program cancellation as well as error exceptions.

In this case it looks like line % data is the part that is expected to throw, rather than .write(), and that probably only throws a single kind of exception.

— Reply to this email directly, view it on GitHub https://github.com/Kattis/problemtools/pull/237#issuecomment-1736296599, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB2ZBKVWOHF75QKKPZIVGPLX4M7CNANCNFSM6AAAAAA5CADBNU . You are receiving this because you commented.Message ID: @.***>

Tagl commented 9 months ago

Oh it seems that IOError and OSError are equivalent in Python 3.

Tagl commented 9 months ago

In this case it looks like line % data is the part that is expected to throw, rather than .write(), and that probably only throws a single kind of exception.

And that is TypeError, So I think this could be changed to catch TypeError and OSError and everything else that could come up is probably a sign of some greater issue.

JoelNiemela commented 9 months ago

In this case it looks like line % data is the part that is expected to throw, rather than .write(), and that probably only throws a single kind of exception.

Ah, you're right. I tested this and it looks like the exception that is handled is KeyError.

I won't catch OSError or TypeError since this would only be caused by write failing, or data not being a dictionary, neither of which are handled in the catch body.