Skip to content

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Oct 4, 2025

This PR modifies the handling of our "custom" handlers (the one not using issue_handlers! or command_handlers!) to a custom_handlers! macro in order for:

  1. the handlers to also be processed in parallel
  2. to use the HandlerError infra, instead of the custom logging

I also took the opportunity to move the macros to a dedicated handlers/macros.rs file.

Based on #2192 (only last 2 commits are different)

@Kobzol
Copy link
Member

Kobzol commented Oct 5, 2025

I'd personally rather have the normal boring code rather than another macro, it's hard for me to follow what's going with the handler inside the macro, and it's also confusing for navigation, recently someone couldn't figure out where a handler was being invoked, because it was called through one of those macros.

But I won't block it if you think it's really better.

@Urgau
Copy link
Member Author

Urgau commented Oct 5, 2025

I also prefer having the "normal boring code", but it doesn't allow handlers to be processed in parallel, nor does it to use the HandlerError infra.

If we wanted the same thing as the macro but without the macro, it would look like this:

let check_commits_fut = async {
    if let Ok(config) = &config {
        check_commits::handle(ctx, host, event, &config)
        .await
        .map_err(|e: anyhow::Error| {
            HandlerError::Other(e.context(format!(
                "error when processing check_commits handler",
            )))
        })
    }
}

// for each custom handler do the same thing as above

let (check_commits_ret, ...) = futures::join!(check_commits_fut, ...);

if let Err(err) = check_commits_ret {
    errors.push(err);
}

That's a lot of boilerplate. Most of them are removed by the macro.

@Kobzol
Copy link
Member

Kobzol commented Oct 5, 2025

I also prefer having the "normal boring code", but it doesn't allow handlers to be processed in parallel, nor does it to use the HandlerError infra.

let mut futs = vec![];

if let Ok(config) = &config {
        futs.push(async { check_commits::handle(ctx, host, event, &config)
            .await
             .map_err(|e: anyhow::Error| {
            HandlerError::Other(e.context(format!(
                "error when processing check_commits handler",
            )))
        }) });
    };

let errors = futures::future::join_all(futs).await.into_iter().filter_map(|res| res.err()).collect::<Vec<_>>();

Doesn't seem that terrible.

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

Successfully merging this pull request may close these issues.

2 participants