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

feat(ffi): add Client::is_room_alias_available function #4193

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

jmartinesp
Copy link
Contributor

This uses the underlying sdk::Client::resolve_room_alias fn to check if a room alias is available for room creation.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@jmartinesp jmartinesp requested a review from a team as a code owner October 30, 2024 15:09
@jmartinesp jmartinesp requested review from poljar and removed request for a team October 30, 2024 15:09
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.

Left some nits, feel free to merge once they are addressed.


/// Checks if a room alias is available in the current homeserver.
pub async fn is_room_alias_available(&self, alias: String) -> Result<bool, ClientError> {
let alias = RoomAliasId::parse(alias)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let alias = RoomAliasId::parse(alias)?;
let alias = RoomAliasId::parse(alias)?;

@@ -1126,6 +1127,23 @@ impl Client {

Ok(())
}

/// Checks if a room alias is available in the current homeserver.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could improve the naming, to something like alias_exists() or alias_is_reserved().

When you say available it could mean two things:

  1. The alias exists and the room can be resolved using this alias.
  2. The alias does not exists and it can be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's the case maybe it's better to modify the existing Client::resolve_room_alias to return Result<Option<ResolvedRoomAlias>>. On Kotlin and Swift I guess the result would be a nullable ResolvedRoomAlias? in a throwing function, so we should be able to handle the 3 states (exists, doesn't exist, something failed).

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a good idea as well, though I imagine that this method could then be a common helper method that uses Client::resolve_room_alias under the hood and converts the result to a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a couple of commits with the changes above in case you want to do another review.

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.88%. Comparing base (494532d) to head (27f5676).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4193   +/-   ##
=======================================
  Coverage   84.87%   84.88%           
=======================================
  Files         272      272           
  Lines       29125    29125           
=======================================
+ Hits        24720    24722    +2     
+ Misses       4405     4403    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

You might want to use the Breaking: trailer instead of the Changelog trailer since this is a breaking change in one of the commits. Looks good otherwise.

Breaking: `ffi::Client::resolve_room_alias` now returns `Result<Option<ResolvedRoomAlias>, ClientError>` instead of `Result<ResolvedRoomAlias, ClientError>`. This allows the client to match the 3 possible cases:

- The room alias exists.
- The room alias does not exist.
- The function failed internally.
@jmartinesp jmartinesp force-pushed the feat/add-is-room-alias-available-fn branch from c26225c to 27f5676 Compare November 4, 2024 15:24
@jmartinesp jmartinesp enabled auto-merge (rebase) November 4, 2024 15:24
@jmartinesp jmartinesp merged commit 6828f93 into main Nov 4, 2024
40 checks passed
@jmartinesp jmartinesp deleted the feat/add-is-room-alias-available-fn branch November 4, 2024 15:38
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