Conversation
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good to me! Two things that you can take or leave
pod/src/list/list_view_mut.rs
Outdated
| /// Returns a mutable reference to an element at a given index. | ||
| /// Returns `None` if the index is out of bounds of the current length. | ||
| pub fn get_mut(&mut self, index: usize) -> Option<&mut T> { | ||
| self.as_mut_slice().get_mut(index) | ||
| } |
There was a problem hiding this comment.
Up to you, but I don't find these kinds of functions that just wrap over slice particularly useful, since someone can just use as_mut_slice() and then whatever function they want afterward.
iter_mut() and sort() and sort_by() could equally be removed
There was a problem hiding this comment.
Personally, I like these. I would keep them. It's a much better devex and feels more native to Vec.
There was a problem hiding this comment.
That's a good point, maintaining all these proxy methods does not scale very well. To keep the nice shorthand though, how about a deref impl? Have a look 👀
pod/src/list/list_view_mut.rs
Outdated
| /// Returns a mutable reference to an element at a given index. | ||
| /// Returns `None` if the index is out of bounds of the current length. | ||
| pub fn get_mut(&mut self, index: usize) -> Option<&mut T> { | ||
| self.as_mut_slice().get_mut(index) | ||
| } |
There was a problem hiding this comment.
Personally, I like these. I would keep them. It's a much better devex and feels more native to Vec.
grod220
force-pushed
the
sorting-methods
branch
from
a8d5892 to
8d29317
Compare
grod220
force-pushed
the
sorting-methods
branch
from
8d29317 to
468d837
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good to me too!
Deref can be tough for downstream developers to reason about, so I would include it with the caveat that there's a chance of removing it later.
| assert_eq!(view_ro.capacity(), capacity); | ||
| assert!(view_ro.is_empty()); | ||
| assert_eq!(view_ro.as_slice(), &[] as &[u32]); | ||
| assert_eq!(&*view_ro, &[] as &[u32]); |
There was a problem hiding this comment.
Seeing this kind of syntax gives me flashbacks to doing weird things with Arcs while learning Rust, but it's not the worst thing in the world, since someone can also call .deref() by hand
Sorry for clarity @joncinque, are you suggesting we add code comments to the Deref impls saying something like: or is that caveat just for us to keep in mind? |
|
Oh sorry, mainly for us to keep in mind |
buffalojoec
left a comment
buffalojoec
left a comment
There was a problem hiding this comment.
Honestly I thought .as_slice() was more intuitive. You could use AsSlice and AsMutSlice.
I also didn't think having a few very common higher-level helpers for convenient devex was a bad idea either.
But I won't hold up the PR, feel free to merge as-is if you both like it better.
|
Let's see how it goes and can revise if we find another API serves actual usecases better. I don't fear the major version bump 😅 |
3 participants
While working on auction mechanics, found myself needing a few new methods in ListView:
This PR adds these methods to the respective trait + mut views.
=======
EDIT: Adding deref trait implementation to ListViewMut & ListViewReadOnly to get the benefits above implicitly.