-
Notifications
You must be signed in to change notification settings - Fork 8
Mc/here now with limit #374
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
Conversation
3ec20ee
to
e3d916d
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.
I had a feeling that in some places a user is forced to specify limit
which I believe shouldn't be true, and we internally set the maximum limit
if it is missing.
I approve it so far, but we still have a discussion about this nextOffset
.
// Check if more results are available | ||
if (res.getNextOffset() != null) { | ||
System.out.println("\nMore results available. Fetching next page...\n"); | ||
|
||
// Fetch next page using the offset from previous response | ||
pubnub.hereNow() | ||
.channels(Arrays.asList("coolChannel", "coolChannel2")) | ||
.includeUUIDs(true) | ||
.limit(100) | ||
.offset(res.getNextOffset()) | ||
.async(result2 -> { | ||
result2.onSuccess((PNHereNowResult res2) -> { | ||
printHereNowResult(res2); | ||
|
||
// Continue pagination if needed by checking res2.getNextOffset() | ||
}).onFailure((PubNubException exception) -> { | ||
System.out.println("Error retrieving hereNow data: " + exception.getMessage()); | ||
}); | ||
}); |
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.
Will it make sense / possible to create local closure which will be called from initial and nested hereNow
calls, but in addition to result
will pass offset
for example and inside if it is not nil
, make another hereNow
calls - it will give user better idea because novice may think that it is ok to nest hereNow
calls as long as possible.
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.
Good point. I will modify this code.
...tlin/pubnub-kotlin-api/src/appleMain/kotlin/com/pubnub/api/endpoints/presence/HereNow.ios.kt
Show resolved
Hide resolved
pubnub-kotlin/pubnub-kotlin-api/src/appleMain/kotlin/com/pubnub/api/PubNubImpl.kt
Show resolved
Hide resolved
pubnub-kotlin/pubnub-kotlin-api/src/jsMain/kotlin/com/pubnub/api/PubNubImpl.kt
Show resolved
Hide resolved
pubnub-kotlin/pubnub-kotlin-api/src/jvmMain/kotlin/com/pubnub/api/endpoints/presence/HereNow.kt
Show resolved
Hide resolved
pubnub-kotlin/pubnub-kotlin-api/src/jvmMain/kotlin/com/pubnub/api/PubNub.kt
Show resolved
Hide resolved
|
||
HERE_NOW_OFFSET_OUT_OF_RANGE( | ||
182, | ||
"HereNow offset is out of range. Valid range is 0 to infinity.", |
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.
Currently we know that up to the 1000
. There definitely will be an upper limit, but it can't be infinity
.
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.
Good catch. I will fix this message. But I guess offset can be higher than 1000 so I would go with
"HereNow offset cannot be negative"
What do you think?
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.
Makes sense!
pubnub.hereNow( | ||
channels = listOf(channel), | ||
limit = 100, | ||
offset = response.nextOffset | ||
).async { nextResult -> | ||
nextResult.onSuccess { nextResponse -> | ||
println("\nNext Page Results:") | ||
printChannelData(channel, nextResponse) | ||
|
||
// Continue pagination if needed by checking nextResponse.nextOffset | ||
}.onFailure { exception -> | ||
println("ERROR: Failed to get next page of presence information") | ||
println("Error details: ${exception.message}") | ||
} |
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.
There is another code for docs where I've left a question about closure reuse.
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.
Good point. I will improve paging.
pubnub-kotlin/pubnub-kotlin-api/src/nonJvm/kotlin/com/pubnub/api/PubNub.nonJvm.kt
Show resolved
Hide resolved
override val offset: Int? = null, | ||
) : EndpointCore<Envelope<JsonElement>, PNHereNowResult>(pubnub), HereNow { | ||
private val log: PNLogger = LoggerManager.instance.getLogger(pubnub.logConfig, this::class.java) | ||
private val effectiveLimit: Int = if (limit in 1..MAX_CHANNEL_OCCUPANTS_LIMIT) { |
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.
We say that it could be between 0
and infinity
(so far as specified in the current codebase), but here we limit it with 1
as the lower limit. We shouldn't let users specify smaller than 0
and larger than the upper limit.
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.
If it is between 1 and 1000 then we accept it. If is out of this range then we set it to 1000.
FYI
"HereNow offset is out of range. Valid range is 0 to infinity.", refers to offset not limit.
What do you think?
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.
But we should accept 0
as limit
. The current condition won't let us do it, right?
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.
Yes, current condition won't let 0 as limit.
Do we want to allow this?
What is a use case for limit set to 0?
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.
With 0
it will give us only occupancy
count:
{
"message": "OK",
"payload": {
"channels": {
"test-channel-353": { "occupancy": 1 },
"test-channel-792": { "occupancy": 4 },
"test-channel-836": { "occupancy": 7 }
},
"total_channels": 3,
"total_occupancy": 12
},
"service": "Presence",
"status": 200
}
Which is something that user also may want to have (just to show how many people are present on the channel).
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.
ok, makes sense. I will allow 0 as limit.
…make behaviour consistent with multi-channel calls.
No description provided.