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

Use gateway url provided in Ready event for resuming #666

Merged
merged 9 commits into from
Oct 4, 2022

Conversation

lukellmann
Copy link
Member

Comment on lines -149 to -150
@Suppress("UNUSED_VARIABLE")
val exhaustive = when (state.value) { //exhaustive state checking
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-exhaustive when statements over sealed types are an error now, so this is no longer needed

Comment on lines +28 to +31
private val resumeOrIdentify
get() = when (val sessionId = resumeContext.value?.sessionId) {
null -> configuration.identify
else -> Resume(configuration.token, sessionId, sequence.value ?: 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resumeOrIdentify combines the previous properties sessionStart, resume and identify so that the session id is only queried once

@lukellmann lukellmann requested review from DRSchlaubi, BartArys and HopeBaron and removed request for DRSchlaubi and BartArys August 11, 2022 17:02
@HopeBaron
Copy link
Member

I reviewed the PR a second time and it looks fine; but it's a draft so I'll be waiting til it's complete

@lukellmann
Copy link
Member Author

lukellmann commented Aug 13, 2022

I made it a draft because of this concern:

I think it might be necessary to reset the context on some other Close events, e.g. UserClose (resuming after the user stopped the gateway seems wrong). I'm new to gateway code, so I might be wrong here, what do you think?

But if you say this isn't needed, then this PR is ready.

@lukellmann lukellmann marked this pull request as ready for review August 13, 2022 12:37
@HopeBaron
Copy link
Member

The resume will not need to modify anything because the reset or not is handled by the loop conditions
all what changed in this PR is where should we connect on resumes not how we should connect so I guess we are good to go if you have tested it and left the bot running for a bit with, say "turn on turn off network to test how would that go

@lukellmann
Copy link
Member Author

lukellmann commented Aug 13, 2022

Problem is that I can't really test rn because they only send the default url until September 12 (which means we connect to the same as normal until then).

@HopeBaron
Copy link
Member

In that case guess there is no need to rush and merge as this may be subject to change

@lukellmann lukellmann marked this pull request as draft August 16, 2022 21:00
@lukellmann
Copy link
Member Author

Converted this back to a draft for now

@lukellmann lukellmann force-pushed the 0.8.x branch 2 times, most recently from a1bb076 to 1e6718a Compare August 17, 2022 12:47
@lukellmann
Copy link
Member Author

I checked rn, I'm still not receiving a session-specific URL. Will try again later today/tomorrow.

@lukellmann
Copy link
Member Author

I checked rn, I'm still not receiving a session-specific URL. Will try again later today/tomorrow.

Still no luck... 😞

@lukellmann
Copy link
Member Author

Still nothing, what should we do?

@lukellmann lukellmann marked this pull request as ready for review October 4, 2022 16:21
@lukellmann
Copy link
Member Author

I was finally able to receive a different url and test it - seems to work as intended

@lukellmann lukellmann merged commit a711c68 into 0.8.x Oct 4, 2022
@lukellmann lukellmann deleted the feature/resume-gateway-url branch October 4, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants