Skip to content

Conversation

@scootermon
Copy link
Contributor

This PR adds type hints to the codebase without making any other changes.
There are a few type errors that get picked up by the likes of mypy, ty, pyright, but resolving those requires changes to the code, which I wanted to avoid in this PR.

I tested the scripts with Python 3.9 (the min version as per the project.toml) to verify it still works.

@qkaiser
Copy link
Contributor

qkaiser commented Sep 1, 2025

That's a good first step. We want to get stronger type checking in place in the scope of #79

We'll try to review this in the coming days, thanks for contributing !

Copy link
Contributor

@e3krisztian e3krisztian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a nice PR, I think it should be merged.

Although I added some comments, those are very minor and kind of nitpicking looking at the size of changes.

Copy link
Contributor

@e3krisztian e3krisztian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!
I have found this new "problem", I have skipped over earlier, but otherwise, it looks very good!

I would split off _LayoutPair(?) from _LayoutInfo, and revert the code change in associate_blocks - only casting the result in the return. I think it would result in new typing problem in associate_blocks, but that should be OK - a reminder to fix it later.

@scootermon scootermon force-pushed the feat/type-hints branch 2 times, most recently from 61db1cf to f8857fb Compare September 15, 2025 12:28
Copy link
Contributor

@e3krisztian e3krisztian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@e3krisztian e3krisztian merged commit 5fe9275 into onekey-sec:main Sep 15, 2025
1 check passed
@scootermon scootermon deleted the feat/type-hints branch September 15, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants