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 a way to add custom state and update sdk #13

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

MTRNord
Copy link
Owner

@MTRNord MTRNord commented Mar 12, 2022

This is heavily building upon what http and axum do. In fact there is a lot of reused code.

This allows for function defined like this:

#[command(help = "`!hello_world` - Prints \"hello world\".")]
pub async fn hello_world<'a>(
    Extension(tx): Extension<mrsbfh::Sender>,
) -> Result<(), Error> {
    let content = RoomMessageEventContent::notice_plain("Hello World!");

    tx.lock().await.send(content).await?;
    Ok(())
}

Downsides:

  • It adds a Mutex to mutable stuff.
  • It needs stuff to be strictly async (I guess that was needed before too?)
  • A little more verbose?

Pros:

  • You can pass anything to it as long as it is defined in your sync handler. (the macro auto extracts the arguments from it)
  • You don't need to pass all things to every command

Fixes #12

Missing is:

  • Getting user supplied things from the sync handler and passing it on.

Tests seem to work:

image

@MTRNord MTRNord added the enhancement New feature or request label Mar 12, 2022
@MTRNord MTRNord self-assigned this Mar 12, 2022

let mut msg = mrsbfh::commands::Message::new();
// TODO insert all the things in the function args
msg.extensions_mut().insert(std::sync::Arc::clone(&args));
Copy link
Owner Author

Choose a reason for hiding this comment

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

@donicrosby This is basically where now your wish comes in. I basically need to do this dynamically for all the things that the function provides when you use the macro. This is currently still hardcoded to only do the config. But the example bot should give you an idea how the api will look like. Which means it should be fairly similiar as before with not too many changes needed

Copy link
Owner Author

Choose a reason for hiding this comment

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

Pushed the rest now. So in theory this PR should work. It compiles but I didnt actually test if it works in practice

Copy link
Owner Author

Choose a reason for hiding this comment

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

One issue is that mutex stuff needs to already be Arc<Mutex<>> to work. It should however then clone the arc as required

Choose a reason for hiding this comment

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

That shouldn't be too difficult, I think that's fairly standard for something like this.

Thanks for the quick fix! I'll test it out later tonight!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sure no problem :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

If you have any bugs feel free to mention them in this PR :) I only ran the example bot as I have no other to test with currently. So there may be stuff i missed

@MTRNord
Copy link
Owner Author

MTRNord commented Mar 12, 2022

Followup issue: #14

@MTRNord
Copy link
Owner Author

MTRNord commented Mar 13, 2022

Some feedback from jplatte:

One thing I noticed is that you seem to have an automatic 'extension' of Arc<Mutexmrsbfh::Sender>. I would recomment instead making the Arc<Mutex<_>> part internal so that the sender is Clone + Send + Sync and you make it an extractor by itself (no Extension wrapping needed)

Extension can still make sense if you want users to be able to add their own context

In the SDK the Ctx type and register_event_handler_context fills the same role

@MTRNord MTRNord changed the title Add a way to add custom state Add a way to add custom state and update sdk Jun 18, 2022
@MTRNord
Copy link
Owner Author

MTRNord commented May 24, 2024

Main missing TODO is now fixing the comments I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for shared global state (similar to Actix's web::Data)
2 participants