search

arcnmx / serde-ini

Windows INI file {de,}serialization
MIT License
19 stars 17 forks source link

Document `Error` #9

Closed bestouff closed 5 years ago

bestouff commented 5 years ago

Apparently serde_ini can't support all data structures. Fine, but would it be possible to know why ? E.g. when serde_ini::to_vec(&item) returns Err(TopLevelMap) it would be convenient to be able to look up in the docs what's the problem.

arcnmx commented 5 years ago

Apparently serde_ini can't support all data structures. Fine, but would it be possible to know why ?

Are you looking for the rationale, or is it just not obvious what TopLevelMap is meant to indicate? This should be added to the documentation, yeah - the README touches on this, mostly just that this serializer/deserializer only supports what the INI format allows - key/value maps with one level of nesting for sections. Any other data type can't really be represented by an INI file without wrapping it into a map/struct in some non-standard way.

So, the error types in particular though could probably use some improvements to be more useful and intuitive...

  1. They should probably be obvious just by their name, so maybe it would help to choose a better name like TopLevelMapExpected or TopLevelMustBeMap or some other variant on that if it wasn't clear?
  2. Better Display/Debug would help too...
  3. The main other non-intuitive error variants:
    • InvalidState is I believe an internal state assertion failure that really should just be made unreachable!() instead.
    • OrphanValue is a limitation of the serializer, it's really not obvious from the name what's going on here, and should be worked around with allocation anyway. Docs pretty important here.
  4. Perhaps TopLevelMap in particular shouldn't even be an error variant. Ideally this would be enforced at compile/typecheck time, because the crate can only support {de,}serialization with certain types (and using it with others is breaking an API contract) but it's not obvious to me whether this is possible.
    1. Could maybe abuse the linker for this purpose? I did this years ago for a libstd backend, but am wary of shipping something like it without being confident that it won't cause problems with any linkers or optimization modes, if it would even work, and not break with erased_serde, etc...
      • paging @dtolnay if available for opinions: tl;dr a top-level serializer that only supports a restricted subset of types can't really be asserted by the type system... Which of a panic, linker error, dedicated Error::Variant, or Error::Custom("don't do this") seems most appropriate? This is similar to any scenario where Impossible is needed, but a restriction for the top-level document/type feels like a slightly more special case over the usual "can't serialize this member type" error?
    2. Just panic instead, because it will never work and there's no point pushing the burden of checking for what should be a compile-time error to users of the crate.
dtolnay commented 5 years ago

I would recommend having an error variant for this and writing a comprehensible message in its Display impl. I think your problem is:

https://github.com/arcnmx/serde-ini/blob/70c65c2c4ae99cd56ff04c3248470c27df708109/src/ser.rs#L39-L43