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

Properly implement DM room creation & retrieval #15

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

fvjosef21
Copy link
Contributor

This is a bug fix for the send_message, get_dm_rooms, and create_dm_room functions.

I also added code to do a lazy check if there are rooms that are on the server, but not on the bot. I am not sure
why this is happening.

I added a call to set the m.direct type on the newly created dm_room. This is not really supported by
the nio-sdk library, which is missing the MDirectResponse type.

Copy link
Owner

@nexy7574 nexy7574 left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple (mainly formatting) things that need doing before this can be merged :)


room = self.rooms.get(response.room_id)
if not room:
raise RuntimeError("DM room %r was created, but could not be found in the room list!" % room_id)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add documentation for the RuntimeError exception into the docstring? only ValueError and MessageException are documented at the moment

await self._send(
nio.DirectRoomsResponse,
'PUT',
nio.Api._build_path( ["user", self.user_id, "account_data", "m.direct"],
Copy link
Owner

Choose a reason for hiding this comment

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

The whitespace is a little odd here - could you run ruff check --fix and ruff format & push on your fork? just for styling consistency

)
if isinstance(result, nio.RoomCreateError):
raise GenericMatrixError("Failed to create DM room", response=result)

dm_rooms = {}
dm_rooms[user_id] = [result.room_id]
Copy link
Owner

Choose a reason for hiding this comment

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

Just being pedantic here but wouldn't it be cleaner to just do dm_rooms = {user_id: [result.room_id]}?

@nexy7574 nexy7574 linked an issue Mar 3, 2024 that may be closed by this pull request
@nexy7574 nexy7574 added the bug Something isn't working label Mar 3, 2024
@nexy7574 nexy7574 changed the title dm_rooms Properly implement DM room creation & retrieval Mar 3, 2024
@fvjosef21
Copy link
Contributor Author

Fixed the comments. Not sure how to submit PR again.

@fvjosef21 fvjosef21 closed this Mar 7, 2024
@nexy7574
Copy link
Owner

No need to re-submit, changes are automatically added to the PR :)

@nexy7574 nexy7574 reopened this Mar 14, 2024
Copy link
Owner

@nexy7574 nexy7574 left a comment

Choose a reason for hiding this comment

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

Looks good!

@nexy7574
Copy link
Owner

All checks passed, merging.

@nexy7574 nexy7574 merged commit d7028e5 into nexy7574:master Mar 14, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Ghost rooms" returned by get_dm_rooms raising LocalProtocolError
2 participants