search

owengage / fastnbt

Fast serde serializer and deserializer for Minecraft's NBT and Anvil formats
MIT License
185 stars 35 forks source link

Add optional feature for compound tags to preserve ordering #87

Closed Esper89 closed 1 year ago

Esper89 commented 1 year ago

NBT data doesn't care about the order of tags, but having tags show up in a consistent order can be useful for debugging and exploring NBT data structures.

I've added a preserve-order feature to the fastnbt crate that replaces the HashMap<String, Value> in compound tags with an IndexMap<String, Value> using a type alias, which I named CompoundMap. I've also replaced all the relevant uses of HashMap with the alias, so everything works the same regardless of if the preserve-order feature is enabled.

These changes are completely non-breaking, since the only change when preserve-order is disabled is a new type alias.

owengage commented 1 year ago

Hi @Esper89, I believe this could cause problems as features must be additive only. If I use fastnbt as a dependency with this feature disabled, I expect the compound type to be a HashMap, if I add another dependency that happens to depend on fastnbt as well, with this feature enabled, it will cause that type to change for my code too.

I feel like this is best solved by a new type similar to Value but with this index-map instead. I don't think we could add it as a generic parameter to the existing Value? The new type would lead to a fairly gross amount of code duplication, however.

Esper89 commented 1 year ago

That's a very good point about additive features, I hadn't thought about that!

I think the only way to properly solve this would be to make CompoundMap into a newtype that doesn't expose the inner type. The HashMap/IndexMap functions would have to be exposed only through this newtype.

That would obviously be a lot of code, and (assuming not all of the HashMap functions are exposed) would limit what users can do with a CompoundMap, so it would be a breaking change.

I'll start working on that—let me know if that isn't something you'd want, or if there are specific HashMap functions that should or shouldn't be exposed.

owengage commented 1 year ago

Changing the Value type in that way would be a breaking change still, I believe. I don't think there's a compatible way to do this with Value.

This sounds like a fairly specific use case, and would require a new type (maybe IndexValue to match IndexMap?). This would probably end up basically being a copy of the existing value type and it's serialize/deserialize implementations, which is a bit unfortunate for maintainability.

Esper89 commented 1 year ago

I think I'll just close this and continue using my fork, then.