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

Youtube Bee (#25) #248

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

Youtube Bee (#25) #248

wants to merge 32 commits into from

Conversation

Mark-Jung
Copy link

Able to listen for Youtube notifications when beehive is exposed to a public url.

Mark-Jung and others added 26 commits April 29, 2019 21:49
This allows Bees to store a state independent from event parameters.
Those states can be checked in filters or used as action parameters
like other values.

Just as examples, Bees could store whether they're currently connected,
the latest sensor read-out, or the timestamp of the most recent sync
as their states.

In Filters you could then check their state:

{{test eq .context.irc.connected true}}
bees/youtubebee/youtube.go Show resolved Hide resolved
bees/youtubebee/youtube.go Outdated Show resolved Hide resolved
bees/youtubebee/youtube.go Outdated Show resolved Hide resolved
bees/youtubebee/youtube.go Outdated Show resolved Hide resolved
bees/youtubebee/youtubefactory.go Outdated Show resolved Hide resolved
added myself as author, replaced hardcoded address, changed error handling, changed event name
@Mark-Jung
Copy link
Author

I incorporated the comments. Please let me know if I should fix anything else! @muesli

Copy link
Collaborator

@CalmBit CalmBit left a comment

Choose a reason for hiding this comment

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

Overall, looking great. This will be really useful once it's ready to go. Just a few small questions and one final nitpick to make sure you're crediting yourself!

Namespace: factory.Name(),
Name: "notification",
Description: "A push notification was sent by the Youtube channel",
Options: []bees.PlaceholderDescriptor{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any other information that can be extracted from YouTube's API? As it stands, this mostly just seems like a generic push, rather than something more actionable. If not, that's absolutely fine - I'm just wondering, so we can broaden it now if possible 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, again, if we can extract more information, then @muesli's suggestion of dividing the current monolith event into multiple events might be a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

I agree how the current monolith event should be split into multiple events so I looked at the notification and the response object provided in the documentation. Unfortunately, I don't think it's possible to divide the notification into three different types of events. I think you can definitely expand this bee for more functionalities such as uploading video, but in the aspect of notification this is as specific as it can get I believe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if the publication/updated date are initially set to the same value for initial publication, but if they were you could check if they were the same, and at least divide them into two - the initial publish and the update.

Copy link
Author

@Mark-Jung Mark-Jung May 21, 2019

Choose a reason for hiding this comment

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

oof good catch. I missed that. Thank you for your insight. I'll commit that this weekend!

* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
* Authors:
* Christian Muehlhaeuser <muesli@gmail.com>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add yourself here too! 😄

Copy link
Author

Choose a reason for hiding this comment

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

haha yup.

@Mark-Jung
Copy link
Author

@CalmBit @muesli I specified the monolithic event into two: new_vid and change_vid. Please let me know if you see anything else I need to fix!

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

Successfully merging this pull request may close these issues.

5 participants