Contain all layer types in an enum #131
Conversation
There was a problem hiding this comment.
Great that you went ahead with this change, and I think it turned out pretty nice!
It's also interesting to see how you can juggle those properties around, to make them part of Layer instead of each specific layer type.
Btw, why is it still marked as draft, even though it appears to compile and test out fine? (Edit: Ah, because you still want to add support for groups as well)
Just because I wasn't 100% sure of the final interface. Group layers will get their own PR, since they weren't implemented even before these changes. |
tests/lib.rs
Outdated
| @@ -21,15 +22,15 @@ fn test_gzip_and_zlib_encoded_and_raw_are_the_same() { | |||
| assert_eq!(z, c); | |||
| assert_eq!(z, zstd); | |||
|
|
|||
| if let LayerData::Finite(tiles) = &c.layers[0].tiles { | |||
| if let LayerData::Finite(tiles) = &c.layers[0].layer_type.as_tile_layer().unwrap().tiles { | |||
There was a problem hiding this comment.
Hmm, what if we would put as_tile_layer on the Layer, so that we could get &c.layers[0].as_tile_layer()?
There was a problem hiding this comment.
Oops, saw this too late
There was a problem hiding this comment.
Imo not a good decision because as_tile_layer only borrows LayerType, not the entire Layer
There was a problem hiding this comment.
Ah, this type of consideration is still new to me. :-)
|
+218 −168... always the same, if you clean and remove lots of duplicate code, you still usually end up with more code! :S |
|
Okay, I swear this is the last time I ask you to re-review :p Edit: Nevermind, build's broken x_x |
Closes #130 & supersedes #53.
Fits all layer types into a single
LayerTypeenum and places all common data (ID, name, properties, etc) into a struct.