@canton7, other event consumers:
We have this event today:
It’s only emitted for folder changes, but it’s very generically named. I’d like to extend it to cover device events (in addition to the existing DeviceConnected / DeviceDisconnected events we already have). E.g.,
Is this going to break things? The differences compared to the existing one would be
- Missing key
- New key
- New possible values in
The proper reaction to this event if it’s not supported is to ignore it, it just adds information that you’ll be fine without if you were fine without it before. I’m hoping that would be the automatic reaction to the missing
folder key if something else specific wasn’t coded…
(By understanding the event you’ll gain more knowledge about what a device is doing, in addition to it’s completion status. Among other things this makes it possible to differentiate between “Syncing (50%)” (it’s working on it) and “Idle (50%)” (it only has half of the files, and seems satisfied with that for the moment).)
Maybe. I’ll have to check the code… I’ll try and find time tonight, but it might take until sometime next week when I’m back home.
Sorry for the delay!
Yeah the missing “folder” key will crash SyncTrayzor currently.
Okay, it’ll have to be a new event type for now then.
(That said, how about ignoring events you can’t parse rather than crashing? ;))
I parse it correctly, but that folder then comes out as a
null, which causes an
NRE much later. Yay for null.
Yeah if I was doing this project professionally I’d have a layer which does sanitisation on everything I receive. That’s a fair chunk of effort to do comprehensively though so I didn’t bother for something which was never supposed to be popular…
From a purely pretty point of view, I don’t like the fact that the same data can represent two different things, depending on whether or not particular keys/values are present. It’s implied that either folder or device is present, but not neither nor both, which smells. An additional field which days what sort of state change the event represents would be nicer…
We’ve handled breaking changes in the past: give me a little bit to get a new release out and move people across to it…
I know how that feels
Oh yes, there’s no rush here, and the proper way may indeed be to have both
DeviceStateChanged events with no ambiguity. The real error may be that the existing event is named just
StateChanged which makes me want to reuse it. Certainly we’ll not intentionally change things in a way that will break trayzor in a patch release.
You’ve successfully shamed me into being more resilient to changes in events…
That’s probably good, although I didn’t mean to bring that much shame on you. Us breaking the contract the API provides can certainly be a good enough reason to crash.