Skip to content

Add inner for NbtLists #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add inner for NbtLists #10

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/tag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,12 @@ impl NbtList {
NbtList(Vec::new())
}

/// Returns a reference to the internal vector of this NBT list.
#[inline]
pub fn inner(&self) -> &Vec<NbtTag> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn inner(&self) -> &Vec<NbtTag> {
pub fn inner(&self) -> &[NbtTag] {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the swift response! That's true, I chose &Vec<T> just to be consistent but when thinking about it of course a slice makes much more sense. That deref flew right over my head, I was consulting on how to access the Vec and the documentation states that inner, inner_mut and into_inner are available for both NbtList and NbtCompound, but I hadn't realized that the inner of NbtList is actually done via a deref. That being said I think an inner wouldn't hurt for consistency and discoverability.

However, given that deref already provides the functionality of a slice, I do see one single use case where a slice wouldn't suffice (although I do admit it is very obscure). And that is when you have a &NbtList and you want to assert that no memory is allocated or that no panic is possible, you would have the option to check Vec::capacity() to see if it would fit before adding it. I do admit it is very obscure. Although one could also add capacity as a member for NbtList to resolve that problem. But I also believe from a consistency point of view having it return the same as the others makes sense.

That being said I really love this library, I think you guys nailed the design and how well and flexible goes together, I'm really fond of using it and love to see it being well maintained. I'd like to hear some comments on this and then will adjust my PR accordingly. Thanks for taking the time!

&self.0
}

/// Returns a mutable reference to the internal vector of this NBT list.
#[inline]
pub fn inner_mut(&mut self) -> &mut Vec<NbtTag> {
Expand Down