-
-
Notifications
You must be signed in to change notification settings - Fork 1
Integrate AppserviceUserIdentity #18
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
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.
Pull Request Overview
This PR integrates AppserviceUserIdentity support for application service requests and introduces the SupportedPathBuilder trait to enable flexible handling of different path builder types (VersionHistory and SinglePath).
Key Changes:
- Introduced
SupportedPathBuildertrait to abstract path builder input handling across different PathBuilder implementations - Updated
send_request_asto acceptAppserviceUserIdentityinstead of&UserIdfor better appservice support - Replaced manual query parameter manipulation with ruma's built-in
try_into_http_request_with_identitymethod - Added
C::RequestBody: AsRef<[u8]>bounds required by ruma's request conversion functions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Refactored request sending functions to use new authentication and path builder inputs; added send_customized_request_as for appservice requests |
| src/http_client.rs | Updated HttpClientExt trait methods to accept path builder inputs and AppserviceUserIdentity; removed deprecated customization methods |
| src/client/builder.rs | Added C::RequestBody: AsRef<[u8]> bounds and updated path builder input from &SupportedVersions to () for SinglePath compatibility |
| src/client.rs | Introduced SupportedPathBuilder trait with implementations for VersionHistory and SinglePath; updated client methods with new bounds |
| CHANGELOG.md | Documented breaking changes and improvements for this release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zecakeh
left a comment
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.
Could we split this into 2 PRs? The first one just upgrades Ruma, and the second one integrates AppserviceUserIdentity.
|
Will do! I'll be offline for a couple weeks but will fix it up afterwards. For the first PR, what version should I upgrade ruma to? |
|
Since we had a new release of Ruma, I opened #19 to upgrade Ruma. You'll just need to rebase your |
ced126a to
8acf4eb
Compare
|
How's this look? Happy to modify, just let me know. All advice is welcomed 😄 |
zecakeh
left a comment
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.
This looks better overall. Could you avoid unnecessary changes like renaming function arguments and lifetimes, to reduce the diff?
5784a13 to
34d4459
Compare
zecakeh
left a comment
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, I just have a couple of nits left.
|
@rsb-tbg hey, is this Copilot review thing something you enabled knowingly? Could you please disable it for your PRs to our project? We do not accept LLM contributions as per our contributing guide. |
|
Hey @jplatte, no I didn't enable anything. I do see the comments from 2 weeks ago from copilot now that you mention it. I'll check my settings and see if I can disable whatever it is. Sorry about that. |
|
No worries. I would assume you can disable it here. |
|
Yup, already turned it off. I'll keep an eye on it for next time. Thanks |
|
I am sorry to tell you this now, but I think that we could now make the diff even smaller by using We just need to upgrade Ruma to the latest commit, and we will make another release before releasing this crate. |
|
No apology necessary. Will check that out now |
b9b5c22 to
5d42df1
Compare
zecakeh
left a comment
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.
You can bump ruma to a git revision for now, so that this PR can pass CI.
zecakeh
left a comment
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, this looks good now. To be able to use ruma as a git dependency for now, we need to relax the cargo-deny config.
You need to modify .deny.toml:
- In the
[bans]section, changewildcards = "warn" - In the
[sources]section, addallow-git = [ "https://github.com/ruma/ruma", ]
zecakeh
left a comment
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 a lot!
This PR adds support for
AppserviceUserIdentityin appservice requests and introduces the SupportedPathBuilder trait to handle different PathBuilder types.Changes:
Breaking changes documented in CHANGELOG.md.