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

Re-visit From impl for native data types found in NBT data format. #169

Merged
merged 5 commits into from
Dec 16, 2022

Conversation

TerminatorNL
Copy link
Contributor

Now no longer returns None but Result<> with the old value if the type did not match the desired type.

Related: #145 (comment)

Now no longer returns `None` but `Result<>` with the old value if the type did not match the desired type.
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

  • The tag module is private to the crate. I think the NbtType trait and the generic type parameter on InvalidTypeError should be removed to keep the interface as simple as possible.
  • InvalidTypeError is missing a std::error::Error impl.
  • If TryFrom<Value> for T exists, it would make sense to me that TryFrom<&Value> for &T and TryFrom<&mut Value> for &mut T should also exist.
  • It is worth looking at serde_json::Value. Notably, it does not have any trait impls to do Value to T conversions. The closest thing is the as_* methods.

To avoid the above problems, I think it would be best to just remove all the TryFrom<Value> for T and From<Value> for Option<T> impls on Value and recreate the needed conversion functions in valence_anvil. Or create an analogue of the as_* functions if that proves sufficient for your needs.

@TerminatorNL
Copy link
Contributor Author

TerminatorNL commented Dec 14, 2022

Have a look at the latest change. I have ditched all generics and hard-coded the value conversions as seen in serde. It created the following methods:

is_*(&self) -> bool
as_*(&self) -> Option<&*>
as_*_mut(&mut self) -> Option<&mut *>
take_*(self) -> Option<*>

IntelliJ IntelliSense works for all of the methods defined in the macro.

@TerminatorNL TerminatorNL deleted the nbt_try_into branch December 14, 2022 21:59
@TerminatorNL TerminatorNL restored the nbt_try_into branch December 14, 2022 22:00
@TerminatorNL
Copy link
Contributor Author

Woops, I wanted to rename the branch. Guess not!

@TerminatorNL TerminatorNL reopened this Dec 14, 2022
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

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

I think the take_* functions should be renamed to into_* since "take" implies that the argument is not consumed. Other than that, looks good to me.

@TerminatorNL TerminatorNL requested a review from rj00a December 15, 2022 23:11
@rj00a rj00a merged commit c73abac into valence-rs:main Dec 16, 2022
@TerminatorNL TerminatorNL deleted the nbt_try_into branch December 16, 2022 11:20
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.

2 participants