-
Notifications
You must be signed in to change notification settings - Fork 258
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
feat(sdk): Add Client::server
#3908
feat(sdk): Add Client::server
#3908
Conversation
b2b8543
to
b1474f4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3908 +/- ##
=======================================
Coverage 84.13% 84.14%
=======================================
Files 266 267 +1
Lines 28005 28027 +22
=======================================
+ Hits 23563 23583 +20
- Misses 4442 4444 +2 ☔ View full report in Codecov by Sentry. |
This patch adds `Client::server`: the URL of the server. Not to be confused with the `Client::homeserver`. The `server` holds some information, like the `.well-known` file, to discover the homeserver. The homeserver is the client-server Matrix API. `server` is usually the server part in a user ID, e.g. with `@mnt_io:matrix.org`, here `matrix.org` is the server, whilst `matrix-client.matrix.org` is the homeserver. This patch also moves the code about homeserver discovery in the `HomeserverConfig::discover` new method. A new struct is introduced to hold the result, to replace a 4-tuples. `Client::server` is also now a `Option<_>` because in the case of `HomeserverConfig::Url`, the server cannot be known. This patch also removes several clones here and there. Finally, this patch updates a test to quickly test the new behaviour. A next patch will introduce proper tests.
This patch moves `client/builder.rs` into `client/builder/mod.rs`. This patch also moves the `HomeserverConfig` type and its siblings (`discover_homeserver`, `UrlScheme` etc.) into its own module `client/ builder/homeserver_config.rs`. This is purely for cleaning up.
b1474f4
to
5fedc09
Compare
This API is tested via `Client` and `ClientBuilder` but it's preferable to unit testing it too, it makes things clearer and cleaner.
dfd78be
to
171e06a
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.
The changes look good, although the distinction between 'server' and 'home server' is a bit difficult to grasp at first. I might have found an issue with the docs though, if I understood the API correctly.
@@ -30,14 +30,14 @@ use crate::{ | |||
/// Configuration for the homeserver. | |||
#[derive(Clone, Debug)] | |||
pub(super) enum HomeserverConfig { | |||
/// A precise URL, including the protocol. | |||
/// A server name URL, including the protocol. |
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.
Should this be a homeserver URL
?
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.
Indeed! I want to rename all these variants in another PR because it is so confusing.
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.
Fixed and rebased.
This patch fixes an error where the `homeserver` is used for the `server` value.
171e06a
to
b073f28
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.
Thanks for the changes!
This patch adds
Client::server
as anOption<Url>
. This should not be confused with thehomeserver
. For example, in the case of Matrix,matrix.org
is the server, andmatrix-client.matrix.org
is the homeserver.This patch should be reviewed commit-by-commit for the sake of simplicity. Code is moved into their own module for clarity.