Have `ParseWebHook` return a event-related "marker" interface

[hashtagchris]

ParseWebHook currently returns an interface{}. I know there's no one method that all webhook events share. But what if we added a "marker" interface and had ParseWebHook return that type? This would make the return type of ParseWebHook a little clearer, and allow consumers to use the same interface in their codebase as they pass events throughout.

Example:

type GitHubEvent interface {
    Event()
}

func (e *BranchProtectionRuleEvent) Event() {}
func (e *CheckRunEvent) Event() {}
func (e *CheckSuiteEvent) Event() {}
...
func (e *WorkflowRunEvent) Event() {}

Exporting the method (Event() in the example above) would be useful for tests. Tests could define struct types that implement the same interface, independent of specific webhook types.

Related

https://github.com/google/go-github/issues/1154

[gmlewis]

Thank you, @hashtagchris . I think I would have to see this to give my opinion. Do you want to put together a PR with test cases demonstrating its implementation and usage? Bonus points if you don't break any existing APIs. :smile:

[hashtagchris]

Thanks @gmlewis, I'll try to put something together. However I think one of the biggest benefits is the stricter type checking during the build. I'm not sure I can capture that in a test case.

Short example

package main

import (
	"fmt"
	"log"
)

func main() {
	// no build error - have to use runtime checks to catch this bad arg 👎
	LogEvent("Oops, I'm passing a string instead of an event")

	// build error 🎉
	LogEventUsingProposedType("Oops, I'm passing a string instead of an event")
}

type GitHubEvent interface {
	Event()
}

type BranchProtectionRuleEvent struct {
	RuleName string
	// imagine several more fields relevant to BranchProtectionRuleEvent here
}

func (e *BranchProtectionRuleEvent) Event() {}

func LogEvent(event interface{}) {
	switch evt := event.(type) {
	case *BranchProtectionRuleEvent:
		fmt.Printf("BranchProtectionRule: %s\n", evt.RuleName)

	default:
		log.Fatalf("Type %T is not a known event type\n", event)
	}
}

func LogEventUsingProposedType(event GitHubEvent) {
	switch evt := event.(type) {
	case *BranchProtectionRuleEvent:
		fmt.Printf("BranchProtectionRule: %s\n", evt.RuleName)

	default:
		log.Fatalf("Type %T is not a known event type\n", event)
	}
}

https://go.dev/play/p/RacH5YLMAo3

[gmlewis]

Ah, I gotcha, @hashtagchris - thank you for the example! So the down-side to this is that we "simply" need to make sure that every event has the magic line func (e *BranchProtectionRuleEvent) Event() {}, correct? Can this be kept up-to-date with go generate ./... ?

If the answer is "yes" (and I think the answer is indeed "yes"), then we could put all these magic lines in a separate auto-generated file with the standard "DO NOT EDIT - See Contributing.md for details", and never have to think about this again, right?

If so, then this makes sense to me to do. Obviously, others are welcome to comment with other opinions.

If there are no objections, then do you want to put together a PR that implements this, @hashtagchris , or would you like to open this up for one of our other contributors to implement?

[hashtagchris]

Yup and Yup!

I may have time this week to contribute changes. Would you have the generator identify the event structs by iterating over the eventTypeMapping values? Or use the AST to find the exported struct types with a name ending in Event?

[gmlewis]

Yup and Yup!

Fantastic! It's yours.

I may have time this week to contribute changes. Would you have the generator identify the event structs by iterating over the eventTypeMapping values? Or use the AST to find the exported struct types with a name ending in Event?

Great question! I'm all for the solution that is easiest to maintain (which usually means "simplest") and the one which seems to be most future-resistant as possible in the hopes that no matter what we do to the code, it will reliably find the events. So I'm leaning toward the former, but am leaving it up to you. We can further comment during the code review. 😁

[hashtagchris]

@gmlewis I made some progress, but won't be able to work on this again for several weeks. I created a draft PR (https://github.com/google/go-github/pull/2477) and I'm open to other contributors taking this change forward.

Thanks for the encouragement and guidance!

[marcelosousa]

I second this proposal.

As a step further, we could group events and provide an higher level API. For example, some of them always have a Sender field. So we have dummy code like:

func getSender(eventPayload interface{}) (string, error) {
	switch payload := eventPayload.(type) {
	case *github.IssueCommentEvent:
		return *payload.Sender.Login, nil
	case *github.PullRequestEvent:
		return *payload.Sender.Login, nil
	case *github.PullRequestReviewEvent:
		return *payload.Sender.Login, nil
	case *github.PullRequestReviewCommentEvent:
		return *payload.Sender.Login, nil
	case *github.PushEvent:
		return *payload.Sender.Login, nil
	case *github.ReleaseEvent:
		return *payload.Sender.Login, nil
	case *github.CheckRunEvent:
		return *payload.Sender.Login, nil
	case *github.CheckSuiteEvent:
		return *payload.Sender.Login, nil
	case *github.IssuesEvent:
		return *payload.Sender.Login, nil
	case *github.ProjectCardEvent:
		return *payload.Sender.Login, nil
	case *github.ProjectColumnEvent:
		return *payload.Sender.Login, nil
	case *github.ProjectEvent:
		return *payload.Sender.Login, nil
	case *github.WatchEvent:
		return *payload.Sender.Login, nil
	case *github.ForkEvent:
		return *payload.Sender.Login, nil
	case *github.CreateEvent:
		return *payload.Sender.Login, nil
	case *github.DeleteEvent:
		return *payload.Sender.Login, nil
	case *github.PublicEvent:
		return *payload.Sender.Login, nil
	case *github.GollumEvent:
		return *payload.Sender.Login, nil
	case *github.TeamEvent:
		return *payload.Sender.Login, nil
	case *github.TeamAddEvent:
		return *payload.Sender.Login, nil
	case *github.StatusEvent:
		return *payload.Sender.Login, nil
	case *github.MembershipEvent:
		return *payload.Sender.Login, nil
	case *github.LabelEvent:
		return *payload.Sender.Login, nil
	case *github.MilestoneEvent:
		return *payload.Sender.Login, nil
	case *github.PageBuildEvent:
		return *payload.Sender.Login, nil
	}
	return "", fmt.Errorf("unknown event type")
}

@hashtagchris: I'll try to help out on the draft PR.