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

Add RoomName struct to ruma_events::room::name. #645

Merged
merged 4 commits into from Jul 2, 2021

Conversation

Frinksy
Copy link
Contributor

@Frinksy Frinksy commented Jun 24, 2021

Resolves: #159

I'm not sure if I'm going in the right direction for this PR. I've changed the constructor's signature for NameEventContent which would be a breaking change.

Also, I'm not sure if creating a RoomName type should fail if the name is an empty String.
There is one test which fails, but that is due to the fact that currently it's possible to create an empty RoomName.

@Frinksy Frinksy requested a review from jplatte as a code owner June 24, 2021 20:34
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Yes, this was definitely expected to be a breaking change. I'll update the base branch accordingly. In the future, please select the base branch to be next for breaking changes (it might also be beneficial to use that as the starting point of your branch).

Re. empty RoomName, I think that should be disallowed. The serde attribute on the field will prevent that from being a deserialization error in that context, but being able to have an empty RoomName instance seems not useful.

crates/ruma-events/src/room/name.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room/name.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room/name.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room/name.rs Outdated Show resolved Hide resolved
crates/ruma-events/src/room/name.rs Outdated Show resolved Hide resolved
@Frinksy Frinksy changed the base branch from main to next June 25, 2021 21:08
@Frinksy
Copy link
Contributor Author

Frinksy commented Jun 25, 2021

I've updated the target branch and rebased my branch onto next.

Also, I've added an #[allow(clippy::exhaustive_structs)] to RoomName since I expect that it will always be just a string.

@DevinR528
Copy link
Member

Also, I've added an #[allow(clippy::exhaustive_structs)] to RoomName since I expect that it will always be just a string.

It also makes it really awkward to use RoomName { 0: raw_string, .. } not that there would be any point in doing that because of all the convenience methods and such. But yeah, I don't think there is any tuple struct with a non_exhaustive attr.

@Frinksy Frinksy changed the title [WIP] Add RoomName struct to ruma_events::room::name. Add RoomName struct to ruma_events::room::name. Jun 26, 2021
@jplatte
Copy link
Member

jplatte commented Jul 2, 2021

Thanks!

@jplatte jplatte merged commit c8280b7 into ruma:next Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add a dedicated RoomName type
3 participants