-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
XRead(Group) Params with allowing block=0 #2305
Conversation
14c794a
to
b7dcaea
Compare
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.
Personally, I think we can do this logic by simply determining whether the block
is greater than 0 or not, and in general I usually think that if a block is equal to 0 or less than 0, you don't need a block, and we can explain that in comment. Using XReadParams
is good too. can avoid having too many arguments for this method. and we can also comment or deprecated void xread(int count, long block, Entry<String, StreamEntryID>... streams);
since the block param is not working. Thanks. : )
Nice suggestion, thanks |
@sazzad16 Can you resolve the conflicts so that we can take this pr here? : ) |
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.
LGTM
private static final String BLOCK = "BLOCK"; | ||
private static final String NOACK = "NOACK"; | ||
|
||
public static XReadGroupParams xReadGroupParams() { |
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.
@sazzad16 I would have consider a different name for xReadGroupParams()
perhaps create()
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.
Seems that static method is not really necessary. Because user can also new Object by none args Constructor
. Maybe we can also keep none args Constructor
private?
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.
@gkorland It is there for method chaining. Any good enough name is okay to me.
@dengliming Empty constructor is still an option to the users if they want to use. I don't to hide it until it is hurting the development process.
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.
@gkorland We are carrying this pattern probably since Jedis 2.10.0. Each of the Params classes have a static method with name at this pattern. Some of already merged PR's for 3.6.0 include this pattern. If we want replace this pattern with something like create()
, we'll always have time to do that. IMHO, we shouldn't keep this PR on hold just for this matter if we agree on the implementation of this PR.
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.
Each of the Params classes have a static method with name at this pattern.
If that is the common pattern then lest leave it like that
Resolves #2277