-
Notifications
You must be signed in to change notification settings - Fork 331
Bump Ruma (breaking) #5337
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
Bump Ruma (breaking) #5337
Conversation
9730498
to
d05861b
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #5337 +/- ##
==========================================
+ Coverage 88.83% 88.86% +0.02%
==========================================
Files 333 333
Lines 91478 91356 -122
Branches 91478 91356 -122
==========================================
- Hits 81264 81182 -82
+ Misses 6387 6350 -37
+ Partials 3827 3824 -3 ☔ View full report in Codecov by Sentry. |
d05861b
to
dc253ad
Compare
dc253ad
to
8423330
Compare
None, | ||
rendezvous_server.to_string(), | ||
None, | ||
&SupportedVersions { versions: Default::default(), features: Default::default() }, |
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 we have an associated const
for empty SupportedVersions
upstream?
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.
In this case, it looks to me like we should call /versions
first, otherwise when this becomes stable we won't know which version of the path to call.
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.
Can't we use the version when support for this endpoint initially was added as the fallback? I guess there's nothing wrong with calling /versions
either since this side will know the homeserver URL anyways.
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.
Ruma doesn't support that currently, only the unstable path is used if no supported versions are provided. See ruma/ruma#2063.
8423330
to
93e2549
Compare
I think you can rebase with |
513eba6
to
68bbd48
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.
Nice, thank you very much for doing the bump. Bonus kudos for the pedantic changelog entries.
I left a question about the usage of the _Custom
variant.
A couple of merge conflicts have snuck in. Could you also please get rid of them.
None, | ||
rendezvous_server.to_string(), | ||
None, | ||
&SupportedVersions { versions: Default::default(), features: Default::default() }, |
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.
Can't we use the version when support for this endpoint initially was added as the fallback? I guess there's nothing wrong with calling /versions
either since this side will know the homeserver URL anyways.
request_config: RequestConfig, | ||
respect_login_well_known: bool, | ||
server_versions: Option<Box<[MatrixVersion]>>, | ||
server_versions: Option<BTreeSet<MatrixVersion>>, |
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.
Nice, makes much more sense than an array.
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
…oints Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
e8ceed0
to
c183fdb
Compare
### Breaking changes: | ||
|
||
- `RoomPreview::info()` doesn't return a result anymore. All unknown join rules are handled in the | ||
`JoinRule::Custom` variant. |
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.
PR number please. Looks good otherwise.
Signed-off-by: Kévin Commaille <[email protected]>
c183fdb
to
02ccdc6
Compare
This is
more or lessready, with all the changes that are required due to breaking changes in Ruma.I still need to fix a few tests. I will probably open other PRs soon to try to make this slightly smaller (but there is not much to do).I tried to split the commits by source of the change, so I recommend to review this commit by commit. However after the first commit the code doesn't compile unless all the other commits are applied, so I recommend to squash everything when merging.
Note that currently Ruma is not ready for a release with those breaking changes, however it should be ready in a few weeks.Ruma should be ready for release when the support for room version 12 is added in the next few days.This is supposed to be part of 3 PRs: