Skip to content

Fix deserialization of redacted aliases events#128

Closed
DevinR528 wants to merge 6 commits into
ruma:masterfrom
DevinR528:redacted-aliases
Closed

Fix deserialization of redacted aliases events#128
DevinR528 wants to merge 6 commits into
ruma:masterfrom
DevinR528:redacted-aliases

Conversation

@DevinR528
Copy link
Copy Markdown
Member

I split the first commit into 2, I got the hang of I think!

@jplatte
Copy link
Copy Markdown
Member

jplatte commented Jul 13, 2020

Nice! :D

Copy link
Copy Markdown
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.

I really don't want us hardcoding specific event types into the macro code. I guess we could always fall back to deserializing from an empty object instead of doing that only for m.room.aliases. That doesn't feel like a very clean solution though. Ideas? ^^°

Comment thread ruma-events/tests/redacted.rs Outdated
@DevinR528
Copy link
Copy Markdown
Member Author

DevinR528 commented Jul 14, 2020

Yeah I kinda figured you wouldn't be thrilled with that. Well after thinking about it all evening yesterday the only thing I came up with was adding another trait method to RedactedEventContent something like has_optional_content() -> bool that has a default impl of returning false?

Also documenting that the RedactedEventContent trait is for internal macro use and users who are extending matrix events otherwise it should be ignored. I almost marked it as doc(hidden) but there are cases where it's needed.

@jplatte
Copy link
Copy Markdown
Member

jplatte commented Jul 14, 2020

Yeah I kinda figured you wouldn't be thrilled with that. Well after thinking about it all evening yesterday the only thing I came up with was adding another trait method to RedactedEventContent something like has_optional_content() -> bool that has a default impl of returning false?

Okay, maybe we should just go at this in fully generality. I think that would mean adding is_empty also, and maybe has_optional_content would instead be a third possible value returned from has_deserialize_fields? As in: make it return enum HasDeserializeFields { Yes, No, Optional }.

Also documenting that the RedactedEventContent trait is for internal macro use and users who are extending matrix events otherwise it should be ignored. I almost marked it as doc(hidden) but there are cases where it's needed.

Actually, since we only need custom_redacted and manual RedactedEventContent implementations for supporting multiple room versions, I'd prefer for it to be #[doc(hidden)] for its methods / associated functions to be #[doc(hidden)], then we can also iterate on that without breaking the public API. Or is there some scenario you can think of where a user would need to implement it manually for a custom event? I would expect all custom events to be redacted fully. (pre-spec would be a different beast, but for that we've now got a different mechanism luckily, thanks to you!)

Copy link
Copy Markdown
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.

Just two doc tiny errors that I will apply, this is looking good.

Comment thread ruma-events/src/lib.rs Outdated
Comment thread ruma-events/src/lib.rs Outdated
@jplatte
Copy link
Copy Markdown
Member

jplatte commented Jul 15, 2020

GitHub is having issues, I rebased/squashed and pushed manually.

@jplatte jplatte closed this Jul 15, 2020
@DevinR528
Copy link
Copy Markdown
Member Author

Cool!

@DevinR528 DevinR528 deleted the redacted-aliases branch May 3, 2021 11:52
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.

2 participants