Candidate/2.8.x #17
Candidate/2.8.x #17
Conversation
| /** | ||
| * Unrecognized replication state. | ||
| */ | ||
| UNKNOWN |
There was a problem hiding this comment.
Would it be prudent here to define this in such a way as it is not the "next status after busy" but rather something like -1 or 999? That would defend against the case where for some reason another status is added in LiteCore that needs to map to the value after busy.
There was a problem hiding this comment.
If I understand what you are saying, it doesn't work like that. These things don't have values... they just are typed values. I could put UNKNOWN at the other end (before STOPPED).
FWIW, though, you can't do something like BUSY + 1.... They aren't fancy names for integers.
There was a problem hiding this comment.
As a specific LiteCore version / commit will be used, what is the cause to get UNKNOWN status? I think, if there is a new LiteCore status, we should map the status to the ActivityLevel (Public API) correctly.
There was a problem hiding this comment.
I thought about this a lot, before I did it, @pasin . You could convince me, though, to revert to throwing an exception.
I think, though, that it is quite reasonable to believe (purely abstractly, now) that there might be times at which the bindings cannot determine the replicators state. It seem quite reasonable, to me, to report an unknown instead of making something up.
In our current implementation, users will never see the UNKNOWN state. After some consideration, though, I think it is not unreasonable to think that they might.
There was a problem hiding this comment.
The platform ActivityLevel's values needs to be mapped with the C4ReplicatorActivityLevel's values.
If the ActivityLevel's value cannot be mapped (e.g. C4ReplicatorActivityLevel introduces a new value), the platform code needs to be fixed to map it correctly or the API needs to be changed to include a new enum value. There shouldn't be an unknown state unless we have a bug.
In our current implementation, users will never see the UNKNOWN state. After some consideration, though, I think it is not unreasonable to think that they might.
Then we shouldn't include it in the public API. This is more like the implementation detail that somehow the platform enum values are not matched with the LiteCore's enum values.
There was a problem hiding this comment.
Fixed. Per Pasin, any unrecognized Core state is now reported as "BUSY".
There was a problem hiding this comment.
This was more to prevent any accidents like with the stopping status, but BUSY vs UNKNOWN I don't much care. LiteCore also defaults to BUSY now, so this is just an extra layer of protection.
|
All the tests have passed: |
|
Added one more commit: changes to AbstractReplicatorConfiguration to make ReplicatorType visible from Kotlin. |
| /** | ||
| * Unrecognized replication state. | ||
| */ | ||
| UNKNOWN |
There was a problem hiding this comment.
The platform ActivityLevel's values needs to be mapped with the C4ReplicatorActivityLevel's values.
If the ActivityLevel's value cannot be mapped (e.g. C4ReplicatorActivityLevel introduces a new value), the platform code needs to be fixed to map it correctly or the API needs to be changed to include a new enum value. There shouldn't be an unknown state unless we have a bug.
In our current implementation, users will never see the UNKNOWN state. After some consideration, though, I think it is not unreasonable to think that they might.
Then we shouldn't include it in the public API. This is more like the implementation detail that somehow the platform enum values are not matched with the LiteCore's enum values.
Preliminary candidate for the 2.8.x release