Be more resilient to additions to ipc interface

[tmerr]

At runtime i3ipc-rs shouldn't depend on i3 version == X. It should depend on i3 version >= X. In theory this should be possible since i3 aims to continually add things to its ipc interface while staying backwards compatible. In order to support this we need to change the way we work with the parsed JSON. It's OK to ask for the value of a field, but it's not OK to ask for the value to lie in a fixed set of possibilities, which we do when constructing many enums in events.rs. Rework this library so it doesn't break after routine i3 upgrades.

[tmerr]

Three options for dealing with unknown values in an enum type T

• Ignore them
• Report them as None in an Option<T>
• Report them as a T.Unknown

I don't like the first since it would imply ignoring the entire data structure containing it. The second is alright but syntactically noisy to use, and it's not obvious what is going on based on the type signature. The explicitly named Unknown variant seems best.

[soumya92]

Is option 3 the path going forward, or is that still up for discussion? Would it make sense to add a string to Unknown for the actual event type?

i3 added a "workspace:reload" event that I would like to avoid crashing on, since it brings down my listening daemon any time I reload i3 to update its configuration.

I'm happy to help move this along, just let me know what I can do :)

[tmerr]

Thanks @soumya92 ! Would you be interested in tackling https://github.com/tmerr/i3ipc-rs/issues/10 ?

As far as this ticket goes,

Is option 3 the path going forward, or is that still up for discussion?

So far I'm leaning toward option 3 but it's totally up for discussion if you have any thoughts on it.

Would it make sense to add a string to Unknown for the actual event type?

I think I like the idea of defining Unknowns without a string, and whenever we hit one to dump the corresponding JSON into a log so that it's easily debuggable. Would that make sense?

[soumya92]

I added a comment on #10, someone else already fixed it in their fork. If possible, you could just merge those changes.

I was just wondering if you had any reservations or thoughts on a different solution before I dove into adding Unknowns everywhere.

I'm fine with defining Unknown without a string. The reason I suggested adding a String to Unknown was to mimic the existing pattern in https://github.com/tmerr/i3ipc-rs/blob/3bcad86a375eb0dc4f041b0dea8fc0ece2632b2b/src/common.rs#L111

I would be slightly concerned about writing to a file, because then we would have to provide some way of configuring where the file goes. It doesn't feel right to have a library create a log file...

[tmerr]

No I wouldn't put much weight on what I did there. If we decide on Unknown we should put that in place of that Undocumented(String). Also, good point about logging, it would be better to use http://doc.rust-lang.org/log which leaves it up to the library user.

If no logging implementation is selected, the facade falls back to a "noop" implementation that ignores all log messages. The overhead in this case is very small - just an integer load, comparison and jump.

[tmerr]

From the sounds of it Unknown + logging would be fine, I would be happy to merge a fix with that 👍

As long as we tell developers how to hook up logging in the README we can still get the debugging info needed to detect missing functionality in i3ipc-rs or i3's docs

[soumya92]

Sounds good! I'll give it a shot and send you a PR. Using the log crate seems fine, especially since we can add a separate target for i3ipc.