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

feat: simplifies the abstraction for Events API messages #18

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

JadedEvan
Copy link
Contributor

@JadedEvan JadedEvan commented Oct 30, 2022

Simplifies the Event abstraction by removing the EventCallbackType and EventType definitions. Through some better utilization in serde we can simplify these type definitions and avoid an unnecessary abstraction that doesn't add much value.

My goal here was to simplify the existing API - as it seemed like there were easier ways to accomplish what the EventType and EventCallbackType structs were trying to accomplish. As an added bonus this also reduces the amount of boilerplate you need to have to unwrap the contents of the Events API message.

Previously to access the contents of the Event one had to have nested match statements

match event.payload {
    Event::EventCallback(event_callback) => match event_callback.event {
        EventCallbackType::AppHomeOpened { user, .. } => {
        // Do something here with user
        },
    },
}

This PR gets rid of this intermediate abstraction and allows you to match directly on the event while preserving the pattern matching in the previous version (matching on the EventCallback::AppHomeOpened type)

 match e.payload.event {
    EventCallback::AppHomeOpened { user, .. } => {
        // Do something here with user
    }
}

Simplifies the `Event` abstraction by removing the `EventCallbackType`
and `EventType` definitions. Through some better utilization in serde we
can simplify these type definitions and avoid an unnecessary
abstraction that doesn't add much value.
@JadedEvan
Copy link
Contributor Author

@Gompei hi there! I'd like to get your thoughts on the approach presented in this PR to see if you're supportive of the changes/direction I'm suggesting. I originally started this work trying to create a PR for #16. I'd like to move forward with better support for the subtype messages in the Events API but I'd like to try and simplify the code before I invest in that approach. Let me know what you think!

@Gompei
Copy link
Owner

Gompei commented Oct 30, 2022

@JadedEvan
Thank you for the PR!

I'd like to move forward with better support for the subtype messages in the Events API but I'd like to try and simplify the code before I invest in that approach.

I have reviewed the changes and agree with what is proposed in this PR. 😄

This PR gets rid of this intermediate abstraction and allows you to match directly on the event while preserving the pattern matching in the previous version (matching on the EventCallback::AppHomeOpened type)

 match e.payload.event {
    EventCallback::AppHomeOpened { user, .. } => {
        // Do something here with user
    }
}

I think it has become very simple and better!

@JadedEvan JadedEvan marked this pull request as ready for review November 2, 2022 11:01
@JadedEvan
Copy link
Contributor Author

Thanks @Gompei - I changed this from a draft PR to a real PR - whenever you're ready to review. This will give me an opportunity to add some better unit tests for the Events API

@Gompei Gompei self-requested a review November 2, 2022 12:08
@Gompei Gompei merged commit 3c50ee8 into Gompei:main Nov 2, 2022
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