Add type annotations by CoolCat467 · Pull Request #519 · ethteck/splat

CoolCat467 · 2026-02-16T03:47:03Z

In this pull request, we add missing type annotations and fix reported mypy type issues.

ethteck · 2026-02-18T11:39:28Z

Hi, thanks for your PR @CoolCat467. I think it would be good to avoid changing the format of the import statements, as this will break usage of splat as a library and/or subrepo unfortunately. Keeping the scope of this to just type annotations would be ideal.

AngheloAlf

Hi! Thanks for your interest in this project!

I'm a bit curious, have you used this repository before? And what game have you tried it for?

Also, CI is pretty angry at this PR, you should take a look at it.

src/splat/disassembler/disassembler_section.py

AngheloAlf · 2026-02-18T11:37:43Z

src/splat/segtypes/common/codesubsegment.py

-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)


This is a pattern we do like a lot, because it allows us to change the parameter list without changing every place where __init__ of an subclass of Segment is overriden. We only name all the parameters of the constructor when we need any of those parameters.
Please revert it.

It starts getting really nasty trying to keep the type arguments correct if I don't change this pattern. I'd like to avoid using Any as much as possible, because it completely eliminates the benefits of type checking.

AngheloAlf · 2026-02-18T11:40:56Z

src/splat/segtypes/common/codesubsegment.py

        "Allows to configure the section before running the analysis on it"

        section = disassembler_section.get_section()
+        assert section is not None


I think it would be nice to include a bit of information about this segment on the second argument of the assert, like the segment name or rom address so it is easier to debug we it failed.
There are a few examples in the repo

src/splat/segtypes/common/group.py

AngheloAlf · 2026-02-18T11:44:15Z

src/splat/segtypes/common/group.py


-    def cache(self):
+    @override
+    def cache(self) -> list[tuple[SerializedSegmentData | list[str], int | None]]:  # type: ignore[override]


I think we should avoid usage of # type: ignore as much as possible, unless we have a good reason for it, because it defeats the point of type hints

AngheloAlf · 2026-02-18T11:53:27Z

src/splat/segtypes/common/group.py

@@ -1,22 +1,28 @@
-from typing import List, Optional
+from __future__ import annotations


Just noted that from __future__ import annotations is deprecated and will be removed in the future, so it would be better to avoid introducing it here
https://docs.python.org/3.14/library/__future__.html#id2

I agree that eventually we will not need to use this as soon as we drop support for older python versions, but until we drop support for like 3.14 or something several years in the future it's perfectly acceptable to use this for having modern type annotations in projects that support old python versions.

src/splat/util/log.py

AngheloAlf · 2026-02-18T11:55:36Z

src/splat/util/options.py

-    yaml: Dict,
-    config_paths: List[Path],
-    modes: List[str],
+    yaml: Mapping[str, object],


Why Mapping instead of dict?

Mapping is co-varient instead of contra-varient or something, unlike dict. See https://stackoverflow.com/questions/52487663/python-type-hints-typing-mapping-vs-typing-dict for more info.

Sure, but we don't really care about other kind of collections. Similarly to how we use list instead of Sequence.
I think we should use dict and keep it simple.

src/splat/util/vram_classes.py

Darxoon · 2026-02-18T21:04:55Z

src/splat/disassembler/disassembler_section.py

+        name: str,
+        rom_bytes: bytes,
+        segment_rom_start: int,
+        exclusive_ram_id: str | None,


the PR makes use of this union syntax a lot but this was actually introduced in python 3.10 (source), unlike splat which (according to angheloalf) supports 3.9

Again, this is not a problem because of using from __future__ import annotations. Anything that's trying to use annotations at runtime uses something like typing.get_type_hints, which deals with string versions of annotations properly.

AngheloAlf · 2026-02-19T11:31:51Z

Well... This PR now tripled in size since yesterday (it was +500 -400, now it is +1600 -1400. yikes).
From a quick look seems like most of those changes are mostly changes in the configuration of the formatter. That kind of changes is a lot more subjective and related to the preference for each project.

I think the formatting changes should be reverted because it would be better to keep the PR focused about type hints and try to make the PR smaller.
Increasing the size of the PR overnight with changes unrelated to the original intention is bad for reviewing, consistency and such.

I'm not saying adding configurations for the formatter is a bad idea, but it adds a lot of noise and makes the PR harder to review. Each config should be discussed and agreed on, so just adding a lot of them makes the discussion harder to have.
Would be fine to move this changes to other PRs, and introduce the changes gradually.

AngheloAlf · 2026-02-19T12:07:57Z

About the overall PR, I think I won't continue to review it until at the very least half the CI checks are passing. Specially the ones that just check if the program itself can still be run or not, which currently doesn't with this PR. After all, we don't merge a PR if it doesn't have all the CI checks passing.

I have limited time for working in hobbies like this one, and I don't want to use it in reviewing an in-the-works PR.
I can still discuss stuff if you want to ask me something.

I think a better approach for this problem would be to split this into multiple, more approachable and smaller, PRs.
For example you could make a PR that enables 1 or 2 mypy lints at a time and address those first, discuss it, and if we like the changes merge and repeat.

btw, you never answered my question. What game are you using this project for? 👀

CoolCat467 · 2026-02-19T15:43:29Z

I'm not really intending on having this reviewed yet, it's not ready, still in draft mode, but I am glad nonetheless to have early feedback. Will probably revert the formatting commit in that case.

I think the idea of spinning this off into several smaller pull requests is a much better idea now that I think about it. In that case, I think I'm going to transition this particular pull request to only get what I've added so far to pass a slimmed down version of the errors to at least get the ball rolling and then start working on others that build on this one.

Not currently using splat in any of my own projects, it just looked like an interesting challenge to add type annotations. With full type annotations, mypy becomes very powerful and can help alert if new code or changed code isn't meshing with how prior things were annotated, and can help discover issues with the program before someone hits them in an edge case at runtime.

This reverts commit 826594c.

CoolCat467 · 2026-02-21T05:02:06Z

Able to get test.py to run successfully now on my machine

ethteck · 2026-02-21T05:39:03Z

I re-ran CI - please fix the remaining errors

WIP add type annotations

b3d80ae

CoolCat467 marked this pull request as draft February 16, 2026 03:47

AngheloAlf reviewed Feb 18, 2026

View reviewed changes

CoolCat467 added 2 commits February 18, 2026 12:32

Add ruff linting rules

0df25a4

Revert import changes

b0aade7

Darxoon reviewed Feb 18, 2026

View reviewed changes

CoolCat467 added 5 commits February 18, 2026 17:28

Run typing-related ruff linting

826594c

Don't use typing.override

1f41416

Workaround for typing.NotRequired for older python

565a48d

Add more missing annotations

62e5232

Support older python without TypeAlias

e4583f9

CoolCat467 added 5 commits February 20, 2026 03:39

Revert "Run typing-related ruff linting"

a1a9eb6

This reverts commit 826594c.

Fix syntax error

20f8c72

Disable strict mode for now

ff18411

Fix a ton of mypy issues

bb1a107

Fix runtime issue

dcc29d4

CoolCat467 added 4 commits February 21, 2026 00:55

Apply ruff formatting

bb1304c

Ruff autofixes

3e340be

Ruff unsafe autofixes

801581c

Fix/ignore remaining mypy issues

00781df

CoolCat467 marked this pull request as ready for review February 21, 2026 07:22

Fix 3.9 support

e475e74

		def __init__(self, args, *kwargs):
		super().__init__(args, *kwargs)

		@@ -1,22 +1,28 @@
		from typing import List, Optional
		from __future__ import annotations

Conversation

CoolCat467 commented Feb 16, 2026

Uh oh!

ethteck commented Feb 18, 2026

Uh oh!

AngheloAlf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngheloAlf Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngheloAlf commented Feb 19, 2026

Uh oh!

AngheloAlf commented Feb 19, 2026

Uh oh!

CoolCat467 commented Feb 19, 2026

Uh oh!

CoolCat467 commented Feb 21, 2026

Uh oh!

ethteck commented Feb 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AngheloAlf Feb 18, 2026 •

edited

Loading