Major change in stream expected version? #331

Activity

1. yreynhout commented on Nov 1, 2019

   

   yreynhout

   on Nov 1, 2019

   Contributor

   More actions

   Are you using 1.2.x's ExpectedVersion with an older client (e.g. 1.1.x)? Or are you shipping the constant as part of another lib which you can't upgrade? What was the unexpected behavior (I suspect the reuse of -1)?
2. semion-akimtsev commented on Nov 1, 2019

   

   semion-akimtsev

   on Nov 1, 2019

   Author

   More actions

   Well, as ExpectedVersion is an integer, the code which relies on this public contract made some assumptions about -1. After it was switched to 1.2.0-beta.6 some of the checks started to return different results.
3. yreynhout commented on Nov 3, 2019

   

   yreynhout

   on Nov 3, 2019

   Contributor

   More actions

   I've never really liked these as a constant nor as an integer - it should be opaque to the consumer and nobody should ever base decisions on the underlying value - it's a deficiency in SSS's type system. Alas, that ship has sailed for SSS, and so here we are.

   👍React with 👍2Reacted by Damian Hickey and Ilya Palkin
4. thefringeninja commented on Nov 4, 2019

   

   thefringeninja

   on Nov 4, 2019 via email

   Contributor

   More actions

   As I mentioned in #332, calling code should not just blindly rely on these magic numbers. Use a value object in your application code and translate before you call sql stream store.

   …

   On Sun, Nov 3, 2019 at 2:06 PM Yves Reynhout ***@***.***> wrote: I've never really liked these as a constant nor as an integer - it should be opaque to the consumer and nobody should ever base decisions on the underlying value - it's a deficiency in SSS's type system. Alas, that ship has sailed for SSS, and so here we are. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#331?email_source=notifications&email_token=AADY7BZ5MEDZNJLMKYFAJE3QR3EBVA5CNFSM4JH4SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5SEFQ#issuecomment-549134870>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADY7BYXK6YIYYHJCAFVZH3QR3EBVANCNFSM4JH4SB2Q> .

   -- Sent from my regular computer http://twitter.com/thefringeninja http://www.thefringeninja.com/
5. semion-akimtsev commented on Nov 4, 2019

   

   semion-akimtsev

   on Nov 4, 2019

   Last edited by semion-akimtsev

   Author

   More actions

   For now it looks to me that we cannot move on with this version as this is a breaking change, because a public API has been changed in a non-backwards compatible way. And it is not captured in this library's version if I would assume that it uses semantic versioning.

   Use a value object in your application code and translate
   before you call sql stream store

   Could you please give an example?
   IMO, since the API exposes a constant, I cannot imagine a way of wrapping it with anything which would not use an if() function of some sort (or directly expose the same value with a different name).

   Aside of that, the ExpectedVersion type/contract mixes unrelated concerns together by introducing EmptyStream and NoStream. These have no relation to version. So I would argue that there is a need to introduce an entity called Stream which would have a set of properties exposed like bool=IsEmpty. NoStream does not fit into both approaches as it is neither a version nor a stream status, but probably is a part of a stream "accessor" interface.
6. thefringeninja commented on Nov 4, 2019

   

   thefringeninja

   on Nov 4, 2019 via email

   Contributor

   More actions

   It would help a lot if you had example code of what it is you are trying to do.

   …

   On Mon, Nov 4, 2019 at 9:52 AM Semion Akimtsev ***@***.***> wrote: For now it looks to me that we cannot move on with this version as this is a breaking change, because a public API has been changed in a non-backwards compatible way. And it is not captured in this library's version if I would assume that it uses semantic versioning. Use a value object in your application code and translate before you call sql stream store Could you please give an example? IMO, since the API exposes a constant, I cannot imagine a way of wrapping it with anything which would not use an if() function of some sort (or directly expose the same value with a different name). Aside of that, the ExpectedVersion type/contract mixes unrelated concerns together by introducing EmptyStream and NoStream. These have no relation to version but to the presence status of a stream. So I would argue that there is a need to introduce an entity called Stream which would have a set of properties exposed like bool=IsEmpty. NoStream does not fit into both approaches as it is neither a version nor a stream status, but probably is a part of a stream "accessor" interface. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#331?email_source=notifications&email_token=AADY7B45QZLNEIBODCBF4LDQR7O75A5CNFSM4JH4SB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6RWHY#issuecomment-549264159>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADY7B3J7T5MIS3HMFCV7CLQR7O75ANCNFSM4JH4SB2Q> .

   -- Sent from my regular computer http://twitter.com/thefringeninja http://www.thefringeninja.com/
7. yreynhout commented on Nov 4, 2019

   

   yreynhout

   on Nov 4, 2019

   Contributor

   More actions

   I agree this is a breaking change in that an existing value was (re)used for a different purpose and constants being copied and embedded into consumers can make this painful. You can only move to this version if you are willing to recompile, yes.