I see that this is modeled after get_item above - this is kind of the old way of doing things. Rather than checking here for each kind of sequence payload, I think it would be best for the list implementations of these to be defined directly in the PyListRef impl.

the implementation probably could be reused for bytearray?

I don't think this is complex enough to warrant the reuse, especially once it's refactored to use some of the newer conveniences. For example:

• We could create a SequenceIndex type that implements TryFromObject that is accepted by list.__getitem__ and list.__setitem__ (similar to what I just did for range, but with an i32 instead of a PyintRef).
• That impl would internally use i32::try_from_object which will raise "cannot fit" exception rather than doing it manually.
• The checking for the sequence type isn't necessary in the concrete classes. It's basically an extra step that's only necessary because of the shared implementation. To me, this screams "wrong abstraction".

It mostly checks the subscript type - the sequence is just used for the error message. (Is there a neat way to construct that error in the caller and avoid passing sequence through at all?)

@cthulahoops did you read my comment above? checking the subscript type seems like a strong candidate for a TryFromObject implementation.

I'm not referring to the trait here (although yeah, tbh I'm not super fond of it and think an inner sequence type shared by list and tuple could be a good idea). I was just referring to get_item and this new del_item helper, because they seem a bit weird and unnecessary to me.

After thinking about this... Maybe it will be better to do it in the simplest way possible (YAGNI and all, even though we know it's going to be also needed for bytearray and more), and worry about reuse after we actually have two users of this.

Ok, so perhaps I jumped the gun implementing this in one night without waiting for some more input.
So, correct me if I'm wrong but the consensus is to ditch the trait and implement it all on PyListRef?

additionally, per @OddCoincidence, suggestion, instead of using a PyObjectRef for the subscript: the implementation of a SequenceIndex enum using an i32 and a PySliceRef with a TryFromObject like the work done on #784 would make for far less explicit type checking and panic! branching.
should I go with this route?

I like these suggestions as in retrospect it seems much cleaner and more in the realm of 'simplest solution that works' that I ascribe to.

pub enum SequenceIndex {
    Int(i32),
    Slice(PySliceRef)
}

impl TryFromObject for SequenceIndex {
    fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
        match_class!(obj,
            i @ PyInt => match i32::try_from_object(vm, i) {
                Ok(t)  => Ok(SequenceIndex::Int(t)),
                Err(e) => Err(e.into()),
            },
            s @ PySlice => Ok(SequenceIndex::Slice(s)),
            obj => Err(vm.new_type_error(format!(
                "sequence indices be integers or slices, not {}",
                obj.class(),
            )))
        )
    }
}

this is what was ment by internaly using i32::try_from_obj correct?

Yeah, that's the right idea! The first arm can be simplified with ?, i.e.:

i @ PyInt => Ok(SequenceIndex::Int(i32::try_from_object(vm, i.into_object())?),

Also note the addition of into_object: try_from_object takes a PyObjectRef so the downcasted i needs to be casted back to object (kinda weird I know but it can be cleaned up later).

could we reuse RangeIndex? We'll need a "int-or-slice" typy for most __getitem__-like functions.

actually, by the time I completed the rework, I realized that, yes, it potentially could have just used RangeIndex I only need to convert to i32 once It came time to check bounds.

fn delindex(self, index: i32, vm: &VirtualMachine) -> PyResult {
        if let Some(pos_index) = self.get_pos(index) {
//...
}

could be

fn delindex(self, index: PyInt, vm: &VirtualMachine) -> PyResult {
        if let Some(pos_index) = self.get_pos(i32::try_from_object(vm, index.into_object())?) {
//...
}

however, I felt it would be best to leave it as is until directed to merge it. it seemed a little strange to be using something called RangeIndex.