The Wayback Machine - https://web.archive.org/web/20210129183321/https://github.com/google/go-github/issues/303
Skip to content

/ go-github

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

Jump to bottom

Deprecate WebHookPayload #303

Open
willnorris opened this issue Mar 16, 2016 · 5 comments
Open

Deprecate WebHookPayload #303

willnorris opened this issue Mar 16, 2016 · 5 comments

Comments

@willnorris
Copy link
Collaborator

@willnorris willnorris commented Mar 16, 2016

The PushEvent type was originally added in June 2013 in beaff13. I then added the WebHookPayload type a month later in 023e8db. I honestly am not sure why I did that, since they are effectively duplicates. Maybe it wasn't clear at that point that we should be using the same types for webhook payloads and activity events? Or maybe I just wasn't thinking.

In any event, #302 reminded me that we've updated PushEvent to include a variant of Repository that has a string Organization filed (see d722750), but the same update was never made to WebHookPayload. Instead of fixing that, I think we should probably just deprecate WebHookPayload and point people to PushEvent instead.
@parkr
Copy link
Contributor

@parkr parkr commented Mar 16, 2016

I think we should probably just deprecate WebHookPayload and point people to PushEvent instead.

Agreed. I'm happy to submit a patch for this. How would you go about deprecating this? Just a comment? Or do you mean to remove this struct? Or alias it, e.g. type WebHookPayload PushEvent?
@willnorris
Copy link
Collaborator Author

@willnorris willnorris commented Mar 16, 2016 •
edited

I was originally just thinking about a comment, but I suspect that aliasing the type should work fine as well. I can't imagine that anyone is actually unmarshalling a push event they've received from GitHub into a WebHookPayload because of #131.

(somewhat ironically, which I just noticed today, #131 is reporting an issue on WebHookPayload, but the commit that fixed it d722750 was patching PushEvent, so the originally reported issue was technically never addressed)
@RobbieMcKinstry
Copy link

@RobbieMcKinstry RobbieMcKinstry commented Mar 8, 2017

Hi, all!
This may be tangential to this issue, but it's at least somewhat related:
since this repo doesn't release tags, deprecating and removing this method is probably a breaking change for someone out there. Like @willnorris said, it's unlikely, but still technically a breaking change if you guys decide to remove it.

I was wondering if there's a plan to mark to version this lib.

Sorry if I'm clouding the wrong channel with this comment. :)
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 8, 2017

@RobbieMcKinstry We have a discussion about that in #376.
@RobbieMcKinstry
Copy link

@RobbieMcKinstry RobbieMcKinstry commented Mar 8, 2017

Thanks! I hadn't seen that issue!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@willnorris @parkr @dmitshur @RobbieMcKinstry