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.

Sign up for GitHub

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

Jump to bottom

feat(sdk): Check server versions after creating the Client #577

Merged
merged 1 commit into from Apr 8, 2022
Merged

feat(sdk): Check server versions after creating the Client #577

merged 1 commit into from Apr 8, 2022

Conversation

jplatte
Copy link
Collaborator

@jplatte jplatte commented Apr 8, 2022

No description provided.

@jplatte jplatte requested a review from a team April 8, 2022 10:36
poljar
poljar approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Let's consider a RwLock, looks good otherwise.

crates/matrix-sdk/src/client/mod.rs Show resolved Hide resolved
@jplatte jplatte force-pushed the jplatte/get-server-versions-later branch from 92f9709 to 584a7b5 Compare April 8, 2022 11:22
@jplatte
Copy link
Collaborator Author

jplatte commented Apr 8, 2022

So I had to update one test that was checking we look at versions after HS discovery. It seemed more important to me to test discovery itself, but maybe we should also have a test to somehow check that the versions are still requested before any other requests (apart from HS discovery)?

@poljar
Copy link
Contributor

poljar commented Apr 8, 2022

So I had to update one test that was checking we look at versions after HS discovery. It seemed more important to me to test discovery itself, but maybe we should also have a test to somehow check that the versions are still requested before any other requests (apart from HS discovery)?

Sure, I don't have anything against additional tests.

@jplatte
Copy link
Collaborator Author

jplatte commented Apr 8, 2022

Me neither, but I'm also not sure how exactly to write that. What I really wanted to ask: Do we need that before merging this PR? 😄

@poljar
Copy link
Contributor

poljar commented Apr 8, 2022

Me neither, but I'm also not sure how exactly to write that. What I really wanted to ask: Do we need that before merging this PR? smile

Ah sorry for being obtuse. This is sufficiently simple so it can go in without the test.

@jplatte jplatte merged commit 1dbb022 into main Apr 8, 2022
@jplatte jplatte deleted the jplatte/get-server-versions-later branch April 8, 2022 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poljar poljar poljar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jplatte @poljar