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

Add EventType consts representing the X-Github-Event header values #3272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MaerF0x0
Copy link

What: Adds type EventType string and an EventType const for every of the types in the maps in github/messages.go

Why: So that we can do switches like

switch eventType {
    case github.EventTypeFork:
        // do stuff
     case github.EventTypeDiscussionComment, github.EventTypeIssueComment:
        // do something with either
}

Notes:

  • It should be non-breaking change
  • Let me know if you want the EventType used more widely including implementing type EventTyper interface{} on each of those structs

@gmlewis gmlewis changed the title adds EventType consts representing the X-Github-Event header values Add EventType consts representing the X-Github-Event header values Sep 19, 2024
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @MaerF0x0!
Two minor tweaks, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

EventType() EventType
}

// EventType corresponds to the X-GitHub-Event: header for Webhooks events and payloads
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// EventType corresponds to the X-GitHub-Event: header for Webhooks events and payloads
// EventType corresponds to the X-GitHub-Event: header for Webhooks events and payloads.

Comment on lines +46 to +48
type EventTyper interface {
EventType() EventType
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is not used anywhere, let's please delete it.
If someone finds this useful in their own code, then it is a simple matter to create it there instead of here.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 19, 2024

Also, please make sure all tests run locally by following step 4 of CONTRIBUTING.md and push any necessary changes to this PR.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsReview PR is awaiting a review before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants