Add SchemaData, SimpleArrayData, SimpleData

[k-mack]

Support kml:SchemaData, which depends on kml:SimpleArrayData and kml:SimpleData.

Closes #33, #34.

[pjsier]

Thanks for the PR! This is looking great, I had one small comment. Other than that, I had a question around typing.

Currently we're a bit inconsistent on how to handle required attributes. A lot of the style elements like LabelStyle and ListStyle require id and load it outside of the attrs HashMap, but it looks like Link has the same requirement and uses attrs.

I'm not sure I see a huge up or downside either way, but seeing the spec for this one I might lean towards setting SimpleData.name directly. What do you think?

[pjsier]

It's a no-op for here now, but for consistency can we add ..Default::default() here as well?

[k-mack]

Adding this triggers clippy's needless_update warning. I can put it in anyways if you'd like.

[pjsier]

Thanks for catching that!

[k-mack]

Thanks for the review! Regarding adding a struct field for required attributes makes sense. I actually started down that path initially. I then started to weigh whether there should be fields for the optional attributes like how the micromata Java KML library does.

I backed out the changes and settled on storing all attributes in a map with the mindset that if I am a user reading the spec and I see that this library exposes a map of attributes, then I know I should be able to do SimpleData.attrs.get("name").unwrap(). Although, this library doesn't do validation yet, so the unwrap isn't safe.

I agree that we should be consistent, so I will add fields for required attributes.

[k-mack]

When reading required attributes, it makes sense to me that we'd want to remove them from the HashMap. If they aren't removed, then they'll be written twice. This happens only with models that can have a mixture of required and optional attributes like SimpleData. See comment in the below snippet for what I am referring to.

    fn write_simple_data(&mut self, simple_data: &SimpleData) -> Result<(), Error> {
        self.writer.write_event(Event::Start(
            BytesStart::owned_name(b"SimpleData".to_vec())
                .with_attributes(vec![("name", &*simple_data.name)])
                // the below line will write `name` again if it is not removed
                .with_attributes(self.hash_map_as_attrs(&simple_data.attrs)),
        ))?;

        self.writer.write(simple_data.value.as_bytes())?;

        Ok(self
            .writer
            .write_event(Event::End(BytesEnd::borrowed(b"SimpleData")))?)
    }

[pjsier]

@k-mack good catch! I think in other examples we’ve been ignoring the other attributes, but I think it makes sense to use remove instead of get when pulling the id attribute to avoid writing duplicates

[k-mack]

I reviewed the spec for LabelStyle, ListStyle, and Link, but I found them all to treat their id attribute as optional. Am I missing something?

[pjsier]

Thanks for these updates! And looking again at the spec you're right about the style attributes, I'll probably update that in a separate PR