Remove Context from FullEvent #2626
Conversation
github-actions
bot
added
framework
| @@ -14,17 +14,17 @@ macro_rules! event_handler { | |||
| ( $( | |||
| $( #[doc = $doc:literal] )* | |||
| $( #[cfg(feature = $feature:literal)] )? | |||
| async fn $method_name:ident(&self, $variant_name:ident { $( | |||
| async fn $method_name:ident(&self, $($context:ident: Context,)? | $variant_name:ident { $( | |||
There was a problem hiding this comment.
Will get to full review later, but why is this | here? Shouldn't the matching work fine without it?
There was a problem hiding this comment.
Ok, apparently not, since both $context and $variant_name are identifiers, the parsing becomes ambiguous. This could still be done without the bar by defining a event_handler_method! macro that generates one method at a time and is called inside event_handler! for each method. It could have a second rule that takes a special @NO-CONTEXT token at the start which then wouldn't generate a ctx: Context parameter.
There was a problem hiding this comment.
I'm not the best at macro_rules syntax, I'd prefer if this somewhat janky syntax is kept for now and a followup PR could correct it if possible.
GnomedDev
force-pushed
the
fullevent-no-ctx
branch
from
e79b8de to
287739a
Compare
| fn update_cache_with_event( | ||
| #[cfg(feature = "cache")] cache: &Cache, | ||
| event: Event, | ||
| ) -> Option<(FullEvent, Option<FullEvent>)> { |
There was a problem hiding this comment.
A lot of the macro invocations here end up using the cache unconditionally, which I guess somehow works, but it still feels weird to pass in an undefined identifier if the cache is disabled.
There was a problem hiding this comment.
It's overall a bad situation, but I don't think passing Context is any better, because it's still an undefined field access in that case.
There was a problem hiding this comment.
Couldn't you just inline the macro invocations and feature-flag them? Btw, I noticed there's calls to if_cache!(event.update(cache)) which are almost equivalent to just update_cache!(cache, event).
There was a problem hiding this comment.
if_cache!(event.update(cache)) returns None if cache is disabled, whereas update_cache!(cache, event) returns (). This difference is needed because otherwise the None would need to be qualified for it's T. I could of course inline the macro invocations, but that would get very repetitive very quickly and make it a lot harder to read.
GnomedDev
force-pushed
the
fullevent-no-ctx
branch
from
ef52b11 to
51afaba
Compare
arqunis
added
enhancement
The macro syntax looks a bit weird as otherwise I run into parsing issues with
ctxgetting parsed the same as a variant name, so I had to put the pipe there as a separator. This is a general clean up as FullEvent is really weird otherwise, and makes genericClient::dataa bit easier (no need for manual clone impl) if we are going to pursue that.