Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cyip10
Copy link
Collaborator

@cyip10 cyip10 commented Sep 9, 2024

@cyip10 cyip10 requested a review from a team as a code owner September 9, 2024 16:23
@cyip10 cyip10 added the python label Sep 9, 2024
Comment on lines 28 to 29
start: Optional[int] = None,
end: Optional[int] = None,
Copy link
Collaborator

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)

Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Collaborator

@jamesx-improving jamesx-improving Sep 24, 2024

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

@Yury-Fridlyand Yury-Fridlyand linked an issue Sep 23, 2024 that may be closed by this pull request
@jamesx-improving jamesx-improving force-pushed the python/integ_cyip10_bitpos branch 2 times, most recently from 76a318b to af8b82b Compare September 26, 2024 16:40
cyip10 and others added 7 commits October 3, 2024 11:16
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: Chloe Yip <chloe.yip@improving.com>
Signed-off-by: James Xin <james.xin@improving.com>
Signed-off-by: James Xin <james.xin@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix bitpos implementation for python and node
3 participants