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

Implement reasons for leaving a room. #663

Closed
wants to merge 2 commits into from
Closed

Conversation

Frinksy
Copy link
Contributor

@Frinksy Frinksy commented Jul 6, 2021

Resolves: #511

Also adds the reason field in the leave_room endpoint as defined in the unstable spec.

However there is a compilation error that I can't figure out with this part:

        /// Optional reason to be included as the `reason` on the subsequent membership event.
        #[cfg(feature = "unstable-pre-spec")]
        #[cfg_attr(docsrs, doc(cfg(feature = "unstable-pre-spec")))]
        #[serde(skip_serializing_if = "Option::is_none")]
        pub reason: Option<&'a str>,

Compilation succeeds with cargo check --all-features but the regular cargo check fails with:

error[E0392]: parameter `'a` is never used
  --> crates/ruma-client-api/src/r0/membership/leave_room.rs:25:29
   |
25 |         pub reason: Option<&'a str>,
   |                             ^^ unused parameter
   |
   = help: consider removing `'a`, referring to it in a field, or using a marker such as `PhantomData`

error: aborting due to previous error

I feel like there something obvious that I'm missing, but the same code snippet worked fine when adding blurhash support. (That is the only other Option<&str> field that is feature-gated that I could find.)

@Frinksy Frinksy requested a review from jplatte as a code owner July 6, 2021 23:22
@DevinR528
Copy link
Member

Is the gated field the only one with a lifetime? I bet the generated struct has a lifetime but without the field it has no need for it.

On my phone so I can't look into to deep.

@Frinksy
Copy link
Contributor Author

Frinksy commented Jul 7, 2021

Is the gated field the only one with a lifetime?

There is this one which isn't gated and also has a lifetime:

        /// The room to leave.
        #[ruma_api(path)]
        pub room_id: &'a RoomId,

@DevinR528
Copy link
Member

Ok I think I see the problem, the RequestBody is getting a lifetime annotation and then no field with a lifetime hmm https://github.com/ruma/ruma/blob/main/crates/ruma-api-macros/src/api/request.rs#L152-L190. I'm not sure how to fix this @jplatte does the expanding macro have access to the feature list? Any ideas come to mind?

Oh wow, there's an ICE also it looks related to this rust-lang/rust#86438 maybe?

Something that helped me when I had macro issues is https://github.com/dtolnay/cargo-expand you have to be in one of the crates, sorry if you are aware of it

cd crates/ruma-client-api
cargo expand r0::membership::leave_room

Copy link
Member

@iinuwa iinuwa 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 needs a changelog. 😉

@jplatte
Copy link
Member

jplatte commented Aug 5, 2021

Finally this works (through #664)! Squash-merged on the command line: 13af2e2

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.

Implement MSC1983 (Reasons for leaving a room)
4 participants