Remove PyObjectPayload enum #653
Conversation
|
It looks like you still need to remove a |
| pub payload: Box<dyn Any>, | ||
| } | ||
|
|
||
| impl Default for PyObject { |
There was a problem hiding this comment.
in what circumstances do we need a default PyObject?
There was a problem hiding this comment.
This was already there, it just can't be automatically derived anymore. It's used in the _nothing() method that it used for the dict, object, and type types. There's a lot of weirdness there that'll need some work.
There was a problem hiding this comment.
I introduced it for defaulting of the new attributes method, and got to simplify _nothing as a side benefit. (I'm not sure I ended up using it for attributes in the end.) It's probably easy to drop Default and just put the default value in _nothing.
Codecov Report
@@ Coverage Diff @@
## master #653 +/- ##
==========================================
- Coverage 41.01% 40.93% -0.08%
==========================================
Files 78 77 -1
Lines 17586 17439 -147
Branches 4509 4561 +52
==========================================
- Hits 7213 7139 -74
+ Misses 8473 8391 -82
- Partials 1900 1909 +9
Continue to review full report at Codecov.
|
|
I'm just realizing near the end of this process that this would have been a good thing to keep a project or milestone for. Oh well, next time! 😆 |
5ac0864 to
053ceb1
Compare
PyObjectPayload enum
PyObjectPayload enum| @@ -181,7 +181,7 @@ pub struct Frame { | |||
| pub lasti: RefCell<usize>, // index of last instruction ran | |||
| } | |||
|
|
|||
| impl PyObjectPayload2 for Frame { | |||
| impl PyValue for Frame { | |||
There was a problem hiding this comment.
I find myself mixed on this changed - probably just because I'm used to Payload now. I'm sure I'll get used to PyValue.
There was a problem hiding this comment.
My thinking was that this would be easier to understand for project newcomers - e.g. it feels like it'd be easier to explain that a PyRef is a reference to some type that is a PyValue.
There was a problem hiding this comment.
I'm always attached to old names :), but I also feel PyPayload better reflects the fact that it is not a Python value, but a rust inner data for a Python value. That being said, we can always change it later.
|
We should look at breaking up pyobject.rs. It has because somewhat unwieldy again. PyContext is probably the most obvious thing to move. |
|
|
| elements.to_vec().get_slice_items(vm, &subscript)?, | ||
| )), | ||
| } | ||
| if sequence.payload::<PyList>().is_some() { |
There was a problem hiding this comment.
We should use isinstance to check on types, not on payload. But we can fix this in another PR.
| impl PyObject { | ||
| pub fn new(payload: PyObjectPayload, typ: PyObjectRef) -> PyObjectRef { | ||
| pub fn new<T: PyValue>(payload: T, typ: PyObjectRef) -> PyObjectRef { |
There was a problem hiding this comment.
It might be a good idea to swap the order of payload and type here.
|
Well, that was a whole lot of changes! |
This removes the
PyObjectPayloadenum, and places theBox<dyn Any>directly inPyObject.PyObjectPayload2trait is renamed toPyValueto better reflect the purpose it serves.