[red-knot] Prefix Type::call and dunder_call with try#16261
[red-knot] Prefix Type::call and dunder_call with try#16261MichaReiser merged 2 commits intomainfrom
Type::call and dunder_call with try#16261Conversation
MichaReiser
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
MichaReiser
changed the title
Type::call and dunder_call with tryType::call and dunder_call with try
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
The renaming of the existing methods to Type::try_call(), Type::try_call_bound and Type::try_call_dunder makes sense to me. I'm not sure I see the motivation for adding the unused Type::call(), Type::call_dunder and Type::call_bound methods, though. They seem like they should be fairly easy to add if we ever need them?
| } | ||
|
|
||
| impl<'db> KnownInstanceType<'db> { | ||
| pub const fn as_str(self) -> &'static str { |
|
I think it helps establish the pattern and it more clearly communicates the idea. It also provides a place to document where they should be used. My last argument is that I'm often too lazy to add methods like this if I have to use them the first time and I only add them if I rewrote the same code like 10 times. Maybe that's a good thing but I want to take that burden from future me. But I don't feel strongly. Removing them just means that we'll have some |
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
I lean towards leaving out the unused methods until we need them, since we should usually encourage people to use the methods that return Results in most contexts anyway. But I also don't feel too strongly :-)
There was a problem hiding this comment.
I like the pattern, but I also think that we should omit methods that we don't currently need (and IMO it is not clear if we will ever need them.) I think the existence of try_call already strongly implies the pattern even if call doesn't exist (yet).
This will cause more rebase pain for @sharkdp on the descriptors PR :)
|
3 participants
Summary
This PR prefixes the
Type::callanddunder_callmethods withtryto make it more explicit that they return aResultand add
callanddunder_callmethods that return the return-type without aResult. These new methods are intendedto be used outside of type checking.
This follows an existing pattern:
The main motivation for this change is #16238 because it is somewhat common to call
booloutsideof type checking -- Making it desirable to have to dedicated methods for it. Splitting the methods also helps improving performance
because we can skip some analysis when we don't care about errors.
Test Plan
cargo test