Correct and complete type annotations in Python bindings#300
Open
dhdaines wants to merge 5 commits intogarvys-org:mainfrom
Open
Correct and complete type annotations in Python bindings#300dhdaines wants to merge 5 commits intogarvys-org:mainfrom
dhdaines wants to merge 5 commits intogarvys-org:mainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The type annotations in the Python bindings were quite incorrect in a number of places.
In various other places they were simply missing!
Also you should not use
from __future__ import annotations, it was a misfeature that is deprecated and will be removed soon, see: https://docs.python.org/3/library/__future__.html#id2This also revealed what appear to be a number of coding errors. In particular you don't need to use
ctypes.pointerin most places where you can just usectypes.byref. I have removed this except in the one case where it is absolutely necessary (inVectorFst.to_byteswhere we have to pass a reference to a pointer, because the pointer itself is getting replaced)Also the various arguments to
Tr.__init__really should absolutely never beNoneexcept in the curious case where one is being created from an existing pointer.It would probably be better to use
Tr.__new__or create a separate class method to reference an existing underlying pointer (also ... how does this interact with gc?) but I have kept the interface the same for now, just added the properoverloaddeclarations and ensured that the code actually does what they say.Also
MutableTrsIteratorisn't a real Python iterator and probably shouldn't define__iter__and__next__if it isn't going to behave like one, but again, that would be a different change.