Skip to content
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.

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

Add remaining is_method_name and as_method_name in NBT Value #595

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alice39
Copy link

@alice39 alice39 commented Jan 28, 2024

This adds the following methods in Value, ValueRef and ValueMut:

  • is_array
  • is_string
  • is_list
  • is_compound
  • as_i8_array
  • as_i32_array
  • as_i64_array
  • as_string
  • as_list
  • as_compound

And this other methods for Value and ValueMut:

  • as_i8_array_mut
  • as_i32_array_mut
  • as_i64_array_mut

Objective

It is a quality change when dealing with NBT Values and friends, so this kind of code

let id = match value.get("some_key")? {
    Value::String(id) => Some(id),
    _ => None,
}?;

can be turned into

let id = value.get("some_key")?.as_string()?;

specially if there are several of those

This adds the following methods in Value, ValueRef and ValueMut:

  - is_array
  - is_string
  - is_list
  - is_compound
  - as_i8_array
  - as_i32_array
  - as_i64_array
  - as_string
  - as_list
  - as_compound
Copy link
Collaborator

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

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

Looks good. CI is failing though.

@alice39
Copy link
Author

alice39 commented Jan 28, 2024

Hmm, it references to some remove function in compound.rs, but I only added methods in value.rs and cargo check and test are okay in my computer, so not sure

@dyc3
Copy link
Collaborator

dyc3 commented Jan 28, 2024

Hmm. Probably a new lint or something.

@rj00a
Copy link
Member

rj00a commented Jan 31, 2024

I am against adding these. IIRC the current methods on Value were added for parity with Java edition's definition of Compound. For instance, as_i32 will return Some if the value is any of the numeric types, not just Int. If we added this, the expected behaviors across the methods would be inconsistent. I do think the docs could be improved to explain this though.

@alice39
Copy link
Author

alice39 commented Feb 1, 2024

I agree about the inconsistency of methods as it is not possible to transmute an array reference into another type with no cloning I kept it like that.

But I've also been thinking of returning an Iterator instead of an array reference, for instance

-pub fn as_i8_array(&self) -> Option<&[i8]>;
-pub fn as_i32_array(&self) -> Option<&[i32]>;
-pub fn as_i64_array(&self) -> Option<&[i64]>;

+pub fn as_i8_iter(&self) -> Option<impl Iterator<Item = i8> + '_>;
+pub fn as_i32_iter(&self) -> Option<impl Iterator<Item = i32> + '_>;
+pub fn as_i64_iter(&self) -> Option<impl Iterator<Item = i64> + '_>;

Now, it will return Some if there exists either an array of bytes, integers or longs with no cloning (which it's great), just like as_<num>.

Though, I'm not sure how I would keep consistent with as_<num>_array_mut methods, the best, that I can think of, to is remove them and stick with matching.

If you like the approach that I mentioned above, then I'm ready to push that commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants