Python: bitpos valkey8#2256
Conversation
| start: Optional[int] = None, | ||
| end: Optional[int] = None, |
There was a problem hiding this comment.
Please update docs (lines 40-41)
There was a problem hiding this comment.
It is possible to define end, but skip start. The command won't fail in that case, but will return incorrect data.
Probably we need to create multiple factory methods to protect us from that.
There was a problem hiding this comment.
As another option - we can keep start mandatory. If user wants to omit start - bitpos should be callsed without options
There was a problem hiding this comment.
Agree with the latest comment: keep start mandatory, and when user want to omit both start and end, they can just omit the entire OffsetOptions, as itself is an optoinal parameter to the actual command fucntions, like bitpos(). We probably want to make the same change to Node as well, to keep consistency.
There was a problem hiding this comment.
You need tests for new command signature with version check
There was a problem hiding this comment.
sorry @Yury-Fridlyand what do you mean here? Do you mean the BYTE|BIT option as part of OffsetOptions? If that's the case I think we already have that, please double check. In case if you mean something else please elaborate.
There was a problem hiding this comment.
New combination of parameters is available only on a specific server version, we need to ensure that test won't fail on older servers.
There was a problem hiding this comment.
Mystery solved: it is the BITCOUNT command which also uses OffsetOptions has the valkey 8 change of end become optional. For BITPOS, end is always optional.
76a318b to
af8b82b
Compare
e1eb6b0 to
8ce266a
Compare
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: James Xin <james.xin@improving.com>
Signed-off-by: James Xin <james.xin@improving.com>
8ce266a to
b1b50af
Compare
valkey-io/valkey#118