Add type hints#55
Merged
Merged
Conversation
Member
|
I think this is a good idea. |
Contributor
Author
|
@henri-hulski I implemented my idea. I had to add a custom descriptor and bound method class to make it work, but it should behave identically to a native |
Member
|
Cool! Looks good to me. |
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.
These type hints are based on my existing stubs, although I decided to change one of the sharp corners, when it comes to defining actions. Currently dectate expects you to only include the parameters you actually will be passed based on your
configandapp_class_argvalues, mostly as a matter of style. This violates LSP, but is technically still safe within the confines of how these methods are invoked.The result is that type checkers will emit incompatible override errors for pretty much every action implementation in morepath and in dectate's tests as well as any custom actions defined by third parties. I begrudgingly put up with those errors for my own code base, but I don't think it's acceptable for a wider audience, so I decided to erase the type of those methods and replace them with
Any. That does bring some drawbacks, but since the only guaranteed safe way to invoke these methods cannot really be expressed by the type system anyway, I think it's fine.There's one more sharp corner when it comes to directive classmethods. Currently there's a notion of being able to call it with only part of the required arguments and then later fill them in via the
DirectiveAbbreviationcontext manager, the incomplete directive is meant to be discarded, without ever being used as a decorator.Here I decided to go the other way, since erasing the signature of every directive classmethod, seems like a really bad idea and there's currently no type modifier to create a partial signature from a complete signature. So if you still want to do this, you will need to either ignore the type error on the incomplete directive or pass in some default value for the arguments you intend to replace in the context manager.
I also thought about adding a
partialattribute to themethodreturned bydirective, so the new recommended way to do this looks like this:So using the erased signature is a conscious decision. That seems like a better trade-off to me. We can still support the old way of doing it, you just may get a type error for a missing required argument when you do it that way.
If you agree that's a good idea, then I can go add that and update the docs where appropriate.
/cc @henri-hulski