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 4 commits 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.

github/messages.go Outdated Show resolved Hide resolved
github/messages.go Outdated Show resolved Hide resolved
@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
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 3, 2024

Also, please resolve all conflicts and push (not force-push) the changes to this PR. Thank you!

@MaerF0x0
Copy link
Author

MaerF0x0 commented Oct 3, 2024

@gmlewis Thanks for the updates. I will be able to get to this in the later half of October. Just wanted you to know I haven't forgotten about, or abandoned this :)

@gmlewis
Copy link
Collaborator

gmlewis commented Oct 3, 2024

@gmlewis Thanks for the updates. I will be able to get to this in the later half of October. Just wanted you to know I haven't forgotten about, or abandoned this :)

Awesome! Thank you, @MaerF0x0, for the update!
No problem. We'll keep the PR open.

@MaerF0x0
Copy link
Author

@gmlewis I rebased my branch on origin/master so it required a force push.

fixed broken test in 4aa32d2

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.01%. Comparing base (2b8c7fa) to head (4aa32d2).
Report is 158 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3272      +/-   ##
==========================================
- Coverage   97.72%   93.01%   -4.71%     
==========================================
  Files         153      172      +19     
  Lines       13390    14848    +1458     
==========================================
+ Hits        13085    13811     +726     
- Misses        215      944     +729     
- Partials       90       93       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 !

I just did a quick search for a random event string in the code base, like so:

$ rg code_scanning_alert
github/repos_hooks_deliveries_test.go
148:	"code_scanning_alert":            &CodeScanningAlertEvent{},

github/messages_test.go
281:			messageType: "code_scanning_alert",

github/messages.go
52:		"code_scanning_alert":            &CodeScanningAlertEvent{},

github/orgs_audit_log_test.go
45:		"events": ["code_scanning_alert"],
143:				"events":          []interface{}{"code_scanning_alert"},

github/event_types.go
1866:// GitHub API docs: https://docs.github.com/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#code_scanning_alert

and noticed that github/messages_test.go still spells out all of these constants explicitly, which seems odd.

I'm just thinking about when the next event gets added, the developer experience of copying that string multiple places is still not fully solved.

I think the other locations are probably fine, but messages_test.go should probably be updated as well.

Thoughts?

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. waiting for reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants