Typetype clean #660
Typetype clean #660
Conversation
Codecov Report
@@ Coverage Diff @@
## master #660 +/- ##
=========================================
- Coverage 40.75% 40.7% -0.06%
=========================================
Files 77 77
Lines 17402 17410 +8
Branches 4528 4522 -6
=========================================
- Hits 7093 7087 -6
- Misses 8409 8429 +20
+ Partials 1900 1894 -6
Continue to review full report at Codecov.
|
11a3f9a to
633a9b0
Compare
| @@ -1548,6 +1551,19 @@ pub trait PyValue: Any + fmt::Debug { | |||
| fn required_type(ctx: &PyContext) -> PyObjectRef; | |||
| } | |||
|
|
|||
| impl FromPyObjectRef for PyRef<PyClass> { | |||
There was a problem hiding this comment.
I was planning on deleting FromPyObjectRef since it overlaps considerably with TryFromObject - could you use the latter instead? Should be more or less the same, just with the unwrap/panic on the caller instead (which we will eventually be able to get rid of).
There was a problem hiding this comment.
No, from the trait's perspective, it can fail. I was referring to the fact that the mro could be a Vec of PyClassRef rather than PyObjectRef so that no downcasting is necessary.
There was a problem hiding this comment.
I thought this might be the controversial bit.
I tried with TryFromObject but it required me to pass the VirtualMachine into a bunch of code that doesn't currently have a VM. The panic should never happen as an object's type should always be a type. I'd be happy with an interface that doesn't require VM but gives me an option/result to deal with?
Will I pass the VM through?
I suppose the alternative would be to make PyObject.typ a PyClassRef, but that's a pretty invasive change.
There was a problem hiding this comment.
Oh I forget it takes a vm - we can work this out later then.
I'd be happy with an interface that doesn't require VM but gives me an option/result to deal with?
This will exist soon! (PyObjectRef::downcast in my prototype)
I suppose the alternative would be to make PyObject.typ a PyClassRef, but that's a pretty invasive change.
It is but I definitely think we should do this at some point.
There was a problem hiding this comment.
Okay, I'll merge. Once we have PyClassRef everywhere this will go away anyway.
No description provided.