-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python: bitpos valkey8 #2256
base: main
Are you sure you want to change the base?
Python: bitpos valkey8 #2256
Conversation
start: Optional[int] = None, | ||
end: Optional[int] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update docs (lines 40-41)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need tests for new command signature with version check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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>
e1eb6b0
to
8ce266a
Compare
valkey-io/valkey#118