RagnarGrootKoerkamp / BAPCtools

Tools for developing ICPC-style programming contest problems.
GNU General Public License v3.0
49 stars 22 forks source link

#102 Part 1: Initial setup for type checking #392

Closed mpsijm closed 1 week ago

mpsijm commented 1 week ago

Took a while, but finally, here's the first batch of changes for #102 :smile:

This MR adds mypy to the pre-commit hooks, which will catch some pesky type errors that will pop up every now and then. pre-commit will try its best to only type-check the changed files, although it may need to reference other files in order to do so. All-in-all, I'm (currently) not bothered by the small extra delay when creating a commit, but let me know if you think otherwise :slightly_smiling_face:

Note that not everything is typed yet (for example, I'd still like to enable the --strict flag at some point), but it's time to at least merge this branch so that (1) others can benefit from type checking and (2) we prevent bug reports like the recent #388, #390, and #391 (this PR also fixes #388). Also, there are some TODOs left in the code, marked with TODO #102:.

Sorry for the huge diff, this is going to be horrible to review :sweat_smile: However, all type errors needed to be fixed before we can enable mypy in the pre-commit hook and the CI. Most changes should be fairly local, though. Also, reviewing this PR one commit at a time should be doable. Some commit messages also have some extra explanation of why certain things happened, so if you're unsure about a certain part of the diff, git blame is your friend :stuck_out_tongue:

Good luck and have fun with the new typings! :smile:

RagnarGrootKoerkamp commented 1 week ago

Mostly LGTM. Didn't have a super close look, and I trust that all the types you put are correct :) So mostly a few small comments about things I didn't understand.

The remaining TODOs you still plan to fix as part of this PR, right?

mpsijm commented 1 week ago

I trust that all the types you put are correct :)

Same, because mypy throws no errors :smile:

The remaining TODOs you still plan to fix as part of this PR, right?

I'd rather merge this PR as it is right now and keep #102 open. The advantage of Python is that it supports gradual typing: not everything needs to be strictly typed if you don't want to. The TODOs are mostly for trying to make types stricter at some later point in time. This means that you now get benefits of type checking in obvious scenarios or when you explicitly define the type. If you don't define types, you don't get type-checked right now, but that's fine :slightly_smiling_face: Another benefit of merging this now, is because I only sporadically have time to work on this. If we keep this open, I will keep getting merge conflicts if other changes are pushed to master :stuck_out_tongue:

RagnarGrootKoerkamp commented 1 week ago

lgtm. feel free to merge

mpsijm commented 1 week ago

Thanks! :smile:

Because it's a large MR: @mzuenni do you still want to take a look, or can we merge? :slightly_smiling_face:

mzuenni commented 1 week ago

looks good overall but I cant check this all XD I think we should just see and if we notice something broke we can still fix it :)

mpsijm commented 1 week ago

That's fair :stuck_out_tongue: I've been using bt from this branch in the past few weeks, should be fine™ :smile: