Conversation
marijanp
force-pushed
the
add-check-events
branch
from
b5849a5 to
66c53b9
Compare
marijanp
force-pushed
the
add-check-events
branch
3 times, most recently
from
2410faf to
9d2a9ac
Compare
kit-ty-kate
force-pushed
the
add-check-events
branch
from
9d2a9ac to
094e748
Compare
kit-ty-kate
left a comment
kit-ty-kate
left a comment
There was a problem hiding this comment.
That looks alright, but i have a couple of comments for completeness sake
| repository: repository; | ||
| sender: user; | ||
| installation: app_installation_event; | ||
| } <ocaml field_prefix="check_run_event_"> |
There was a problem hiding this comment.
The documentation says this type also contain an (optional?) organization
There was a problem hiding this comment.
I have never received an organization so I could not verify if the existing type fits the one we receive. But I'm going to add an optional organization field now.
lib_data/github.atd
Outdated
| check_run: check_run; | ||
| repository: repository; | ||
| sender: user; | ||
| installation: app_installation_event; |
There was a problem hiding this comment.
Shouldn't this be optional?
| ] <json open_enum> | ||
|
|
||
| type check_run_event = { | ||
| action: check_run_action; |
There was a problem hiding this comment.
with the requested_action action, this type also has an (optional?) requested_action field
There was a problem hiding this comment.
the action might also be optional but i'm not sure
There was a problem hiding this comment.
If action = requested_action then the action field is required is how I understand the docs. With other action types it is not marked as required
There was a problem hiding this comment.
I see what you mean now: you say if the action type is not the requested_action variant then this field is an optional. But the comments clearly say that there is a value being set to created, completed, rerequested. I have also never received a webhook without these being set.
The docs are really awkward, would you prefer to proceed with an option or is this ok?
| repository: repository; | ||
| sender: user; | ||
| installation: app_installation_event; | ||
| } <ocaml field_prefix="check_suite_event_"> |
There was a problem hiding this comment.
same for this one, an organization field might be there
There was a problem hiding this comment.
The optional enterprise field is also available
There was a problem hiding this comment.
We don't have an existing enterprise type and I also never received a webhook to be able to verify the return type.
There was a problem hiding this comment.
I would add the enterprise field in the future on demand
marijanp
force-pushed
the
add-check-events
branch
from
094e748 to
3af9935
Compare
marijanp
force-pushed
the
add-check-events
branch
from
3af9935 to
acb362c
Compare
|
@kit-ty-kate can I get a re-review? |
2 participants
Added two missing webhook events: