search

mm21 / trilium-alchemy

Python SDK and CLI toolkit for Trilium Notes
GNU Affero General Public License v3.0
10 stars 1 forks source link

Implement typing using pyright #11

Closed jwhonce closed 1 year ago

jwhonce commented 1 year ago

This includes work on making EntityIdDescriptor usage pass typing checks.

The changes here remove a number of warnings and errors, the issue I have not resolved fields like note_id that are "str | None" in the Mixin and "EntityIdDescriptor | str | None" in the class Note.

I use the command:

mypy trilium_alchemy/core/note/note.py |grep note.py |less

trilium_alchemy/core/note/note.py:605: error: Incompatible types in assignment (expression has type "EntityIdDescriptor | str | None", base class "Mixin" defined the type as "str | None") [assignment]

to review mypy errors and warnings.

The pyproject.toml changes are just for research, they will be rolled into a poetry setup if this work progresses.

mm21 commented 1 year ago

Thanks for doing this, it's a great start!

For Note descriptors which are defined as fields in Mixin, I think we'll just need to use # type: ignore. If we changed the typing of the Mixin field that would be reflected in the API, so I don't see a way around it.

There are some genuine issues exposed too, like Mixin methods accessing fields defined in Note. To fix this I'd like to do some refactoring to move the relevant code and data structures out from Note to Mixin.

Do you mind if I make those changes on this branch? Then once note.py is error-free we can merge this PR. Alternatively I could create a central branch for this effort and merge your changes.

jwhonce commented 1 year ago

Please choose the method that is easiest for you. I am happy to putter and would hate to hold you up.

mm21 commented 1 year ago

Looks like this fixes all the typing issues in note.py - please advise if you see any problems. Otherwise I'll go ahead and merge to main.

jwhonce commented 1 year ago

pyright is reporting some issues. I will put together a commit to clean them up.

mm21 commented 1 year ago

Sounds good, thanks. I installed pyright and looks like some may require more refactoring, so feel free to leave those if they look like a lot of work and I'll work on them. There's at least one functional issue which would have come up during testing (I haven't done any testing yet but will prior to merging it).

mm21 commented 1 year ago

This fixes all the pyright issues in note.py for me - please advise if you see otherwise. The tests are passing now as well, so I'll go ahead and merge it. This is a great start and I think it's a reasonable goal to have the whole project comply with pyright. Only 523 errors, 11 warnings to go!

I think there could be value in having MyPy as well, but I'll work on that separately - I'm interested in adding a type coverage badge.

This gave me a newfound appreciation for such tools - it exposed some design flaws and forced me to come up with a better, more clear one. There are some real improvements to the design and code clarity I think. Thanks for taking the initiative!

mm21 commented 1 year ago

I forgot to re-run pyright after fixing the runtime issues, but fixed again in main.