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

Create bee Events with less Effort #284

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

penguwin
Copy link
Collaborator

@penguwin penguwin commented Feb 20, 2020

This adds a new createEvent() method on the implemented Bees which handles back a filled Event-Struct for the given Eventname and map of Placholder values.

The map for the Placeholders should be structured like this:

properties := map[string]interface{}{
          "event0": val0,
          "event1": val1,
          "event2": val2,
          // ...
 }

With a map like this you can simply trigger an event like so:

func (mod *MyBee) TriggerEvent(name string, properties map[string]interface{}) {
    mod.evchan <- mod.CreateEvent(name, properties)
}

I also demonstrated the usage in openweathermapbee/event.go and in the ircbee.go and as you can see this reduces the amount of code we have to write.

@penguwin penguwin requested a review from muesli February 20, 2020 18:15
Bees can now create Events based on the name and a map containing the
Placeholders with Names and Values.
To do this we use the Event&Placeholder-Descriptor information which given in
the Bees Factory Namespace.
This will lead to a much shorter declaration of occuring Events in the Bees
implementation.
Using the newly implemented CreateEvent-Method which leaves us with less
boilerplate code for creating the Weather events.
Another example for the usage of the new CreateEvent method which comes in
pretty handy.
@penguwin
Copy link
Collaborator Author

penguwin commented Feb 20, 2020

force pushed some fixed typos in my comments

@rubiojr
Copy link
Collaborator

rubiojr commented Mar 19, 2020

It's a matter of preference, but I'm ok with the existing code if that helps. It's explicit, making it easier to understand what kind of types are being sent in each event when reading the code. A bit less magical.

@penguwin
Copy link
Collaborator Author

I agree that the existing code is less magical and also more explicit about the types. Yet the EventDescriptor already contains the type-information for each placeholder which makes it a bit redundant to have to also declare it for every placeholder in the Event itself.

On second thought, I realized that not using the code from this pr would be in favour of the KISS principle - there should be only one consistent way to declare the events (even if it's more verbose). Therefore I'd also say that we should rather not merge this pr. Any final thoughts @muesli?


p.s. after looking at the code again I also noticed that I should probably break the loop in CreateEvent after appending each Placeholder to the Event - and also return an error if no placeholders were created instead of just returning an empty event.

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