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 support for CreateEventPayload and DeleteEvent payload (#1646) #1932

Merged

Conversation

GMouron
Copy link
Contributor

@GMouron GMouron commented Jan 22, 2019

Hello, I tried to do the same as was done in #1732
Please tell me if anything is missing.
Cheers

@ryangribble
Copy link
Contributor

Hi @GMouron, thanks for this! It looks good although one thing I noticed when checking the docs is that the create event payload has master_branch and description fields as well, so it would seem a good idea to add them to the response model in this PR. What do you think?
https://developer.github.com/v3/activity/events/types/#createevent

/// <summary>
/// The object is of type branch
/// </summary>
[Parameter(Value = "branch")]
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: missing a whitespace line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :)

@GMouron
Copy link
Contributor Author

GMouron commented Jan 30, 2019

Hi @GMouron, thanks for this! It looks good although one thing I noticed when checking the docs is that the create event payload has master_branch and description fields as well, so it would seem a good idea to add them to the response model in this PR. What do you think?
https://developer.github.com/v3/activity/events/types/#createevent

Hello, yes, it mentions those 2 properties, however, the doc mentions that webhooks do not receive repository creation events :

Note: webhooks will not receive this event for created repositories. Additionally, webhooks will not receive this event for tags if more than three tags are pushed at once.

And when I see the explaination of those 2 fields :

master_branch | string | The name of the repository's default branch (usually master).
description | string | The repository's current description.

as well as the content of the events sent by github for the creation of a branch which does not contain them, I've decided to omit these here since, from my understanding, they will never be part of the payload we receive

@GMouron GMouron force-pushed the AddCreateAndDeleteActivityPayloads branch from fe7ec83 to 639830e Compare January 30, 2019 14:02
@ryangribble
Copy link
Contributor

as well as the content of the events sent by github for the creation of a branch which does not contain them, I've decided to omit these here since, from my understanding, they will never be part of the payload we receive

Octokit.net uses these payload response classes for the results from "Activity Event" Api, which does include repository events and thus does include these fields.

The response classes can also be used to deserialise webhook payloads, although we dont natively have support for webhooks, so users doing this would be having to call the SipleJsonSerializer themselves currently.

So I think the extra fields should be included, since the current main supported use case in Octokit (the events API) would use them. They would then just be null for anyone using them from a webhook perspective.

When we implement native webhook support we can consider whether to re-use these payload classes or have new ones specifically for the webhook payloads

@GMouron
Copy link
Contributor Author

GMouron commented Jan 31, 2019

OK, I'll be adding them then :)

@GMouron GMouron force-pushed the AddCreateAndDeleteActivityPayloads branch from 639830e to aa5b540 Compare January 31, 2019 09:38
@GMouron
Copy link
Contributor Author

GMouron commented Jan 31, 2019

And done :)

@ryangribble
Copy link
Contributor

release_notes: Support CreateEvent and DeleteEvent payloads, using new response models CreateEventPayload and DeleteEventPayload

@ryangribble
Copy link
Contributor

Thanks @GMouron! (we have quite a few payload events that aren't implemented - it would be great if you wanted to add more!)

LGTM

@ryangribble ryangribble merged commit 47c38bf into octokit:master Jan 31, 2019
@GMouron
Copy link
Contributor Author

GMouron commented Jan 31, 2019

Thanks @GMouron! (we have quite a few payload events that aren't implemented - it would be great if you wanted to add more!)

LGTM

Hi, I might take a look into that a bit later but I really needed those ones and I have to first take care of the code that depends on them 😄

@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants