Implement Tags in rbx_types, rbx_binary, rbx_xml, and rbx_dom_lua #199
Conversation
rbx_xml/src/types/tags.rs
Outdated
|
|
||
| fn read_xml<R: Read>(reader: &mut XmlEventReader<R>) -> Result<Tags, DecodeError> { | ||
| let value = reader.read_base64_characters()?; | ||
| Ok(value.try_into().unwrap_or_default()) |
There was a problem hiding this comment.
I wanted to use map_err and the question mark operator here but couldn't get it to compile :/
rbx_types/src/tags.rs
Outdated
| pub fn as_vec(&self) -> &Vec<String> { | ||
| &self.tag_list | ||
| } |
There was a problem hiding this comment.
You never want to return &Vec<T> in Rust. How about fn as_slice(&self) -> &[String]?
rbx_types/src/tags.rs
Outdated
| impl From<Vec<&str>> for Tags { | ||
| fn from(tag_list: Vec<&str>) -> Tags { | ||
| Self { | ||
| tag_list: tag_list | ||
| .iter() | ||
| .map(|tag_name| String::from_str(tag_name).unwrap()) | ||
| .collect(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure we need this; users can convert to Vec<String> first themselves:
tags.into_iter().map(String::from).collect()If we wanted this, we would be better off implementing FromIterator to get a bunch of containers for free.
rbx_types/src/tags.rs
Outdated
| impl From<&Tags> for Vec<u8> { | ||
| fn from(tags: &Tags) -> Vec<u8> { | ||
| let tag_list = &tags.tag_list; | ||
| let mut buf: Vec<u8> = Vec::with_capacity(tag_list.capacity() + tag_list.len()); | ||
|
|
||
| for tag_name in tag_list.as_slice() { | ||
| buf.extend_from_slice(tag_name.as_bytes()); | ||
| buf.extend_from_slice(b"\0") | ||
| } | ||
|
|
||
| buf | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels pretty weird to have as part of From. If we want this method, we should write an explicit method for it.
This can also be a one-liner, which makes me think we should consider writing that inline when we need it in rbx_xml and rbx_binary:
fn encode_tags(tags: &[&str]) -> Vec<u8> {
tags.join("\u{0000}").into_bytes()
}
rbx_types/src/tags.rs
Outdated
| impl TryFrom<Vec<u8>> for Tags { | ||
| type Error = FromUtf8Error; | ||
|
|
||
| fn try_from(buf: Vec<u8>) -> Result<Tags, FromUtf8Error> { | ||
| let len = buf.len(); | ||
| let mut tag_list = Vec::new(); | ||
| let mut count = 0; | ||
|
|
||
| while count < len { | ||
| let tag_name: Vec<u8> = buf | ||
| .iter() | ||
| .skip(count) | ||
| .copied() | ||
| .take_while(|element| { | ||
| count += 1; | ||
| *element != 0 | ||
| }) | ||
| .collect(); | ||
|
|
||
| tag_list.push(String::from_utf8(tag_name)?); | ||
| } | ||
|
|
||
| Ok(Self { tag_list }) | ||
| } | ||
| } |
There was a problem hiding this comment.
You can use slice::split and save a lot of effort here.
Similar to the From implementation, this should probably be its own method instead of a generic trait implementation.
rbx_types/src/tags.rs
Outdated
| serde(transparent) | ||
| )] | ||
| pub struct Tags { | ||
| tag_list: Vec<String>, |
There was a problem hiding this comment.
This could have a general name like members.
|
rbx_xml has no great way to discern if a |
|
Never mind on that - we can add a conversion from |
|
Simply adding the conversion wasn't enough for rbx_xml to write |
For simplicity, this is a Vec<String>. We might want to revisit this once #112 is explored further.
This abuses the fact that we can write *anything* for the tag and Roblox will still read the property correctly.
Not ideal... but I'm not sure if we want to mess with rbx_xml's reflection capabilities just yet.
kennethloeffler
changed the title
There is currently no special representation for
CollectionServicetags in rbx-dom. To address this, this PR adds the new typeTagsto rbx_types. It is a wrapper overVec<String>that provides conversions between byte arrays andTags. It changes the type ofInstance.TagsfromBinaryStringtoTagsand makes rbx_binary and rbx_xml write to and read from this property usingTags. This makes it possible for consumers like Rojo to expose a more friendly interface forCollectionServicetags rather than requiring users to provide a base64 string.