Big type hinting and modernization upgrade #147
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello! So, a bit of explaining is in order. I'm the creator of Rich-Pyfiglet and Textual-Pyfiglet, which are implementations of Pyfiglet for Rich and Textual respectively. Rich-Pyfiglet is a fork of Pyfiglet (you can see in the forks list) and then Textual-Pyfiglet uses this fork.
So, I've been working with Pyfiglet for a while. Really big fan. I had noticed in this post on March 24 that you said you were open to PRs for type hints. So thought I would like to tackle this challenge. My two pyfiglet packages pass Pyright and MyPy in strict mode so I set out to try to apply that onto the Pyfiglet package itself.
This doesn't completely add type hints to everything but it does tackle the vast majority of them, basically everything I could figure out easily without needing to dig too deep into the internals. Important to note that there were absolutely no changes to run-time behavior in any way. Aside from a couple standard lib imports (annotations, typing.Any, typing.Optional), every single modification is just adding type hints, except one:
There is also a place in the code where the two uses of the
property()
function were replaced with the @Property decorator. This has been in python since the 2.x days so I thought this is a reasonable modification. I suppose it technically counts as a refactor, but hardly ;p. Aside from that, there was no refactoring done.I have ran the unit tests and confirmed that everything still works perfectly:
pyfiglet/test.py
pytest -vv
For some further evidence that that my changes are good, I'd like to present two files as well that are attached to this post:
pyright_old.txt
andpyright_new.txt
.pyright_old.txt
pyright_new.txt
These results show running Pyright on strict mode. In pyright_old.txt, this shows running Pyright on the original version without any modifications. At the bottom of the file you can see:
But if you peek inside pyright_new.txt, you will see this:
That's about an 82% reduction in Pyright warnings. All the public API points are covered, and I also added a
py.typed
file. So this should now be fine to import and use in other programs without needing any # type: ignore comments.For the record, I also added basedpyright (https://docs.basedpyright.com/) in the dev-requirements.txt, which is an open-source fork of Pyright. I personally prefer it for this kind of work over MyPy because the strict mode will flag every single thing that does not have a type hint, whereas MyPy does not do this and there's no simple way to make it do it that I'm aware of. Adding basedpyright of course required a config file so there's also a pyrightconfig.json file. This will allow any of you that wants to to use it and see the results for yourself.
But, please do look over my work and make sure I didn't make any mistakes. I have not had a second set of eyes overview this. If there's any modifications you want or you'd like me to reduce the scope for a first PR, that's totally fine, just let me know! Hope you like it.