serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Remove `CacheAndHttp`, plus cleanup of dispatch

Open mkrasnitski opened this issue 3 years ago • 4 comments

The CacheAndHttp struct has too similar a name to the CacheHttp trait, which might cause confusion. Therefore, I've removed the struct, and inlined its fields where it was used before. In addition, cleaned up some of client/dispatch.rs, for example the dispatch and handle_event functions now take a Context as parameter (to reduce cloning), and dispatch is now a regular async fn, rather than returning a BoxFuture. Also introduced an impl of Debug to Context so that the tracing instrumentation on handle_event remains unchanged.

mkrasnitski avatar Sep 02 '22 13:09 mkrasnitski

Is it worth the churn? [referring to the "removing CacheAndHttp" part]

kangalio avatar Sep 02 '22 14:09 kangalio

This PR also seems to be multi-purpose, and last time I tried cleaning up this code in a multi-purpose commit everything broke. I believe this should be split up into separate PRs (cleaning up dispatch, possibly removing cacheandhttp, etc)

GnomedDev avatar Sep 02 '22 14:09 GnomedDev

I made my best attempt not to touch any of the caching logic, just simplifying and cleaning things up. I tested with some example bots and it seems to work fine, but you're welcome to clone the branch and test some more substantial code with it.

As for justification, these two fields were wrapped (I presume) to hide the feature-flag on the cache, in an attempt to please the compiler when building without cache enabled. But the struct itself is rather meaningless otherwise, and only adds mental overhead. Plus, the CacheHttp trait abstracts over pretty much the exact same thing, but is useful in more places.

mkrasnitski avatar Sep 02 '22 14:09 mkrasnitski

For #2138, I was thinking of storing CurrentUser in CacheAndHttp as a third field. Could we keep CacheAndHttp for now to keep that possibility open?

kangalio avatar Sep 11 '22 21:09 kangalio

Going to split this up into two separate PRs. Closing for now.

mkrasnitski avatar Oct 01 '22 17:10 mkrasnitski