Skip to content

Conversation

marijanp
Copy link

@marijanp marijanp commented May 17, 2025

@marijanp marijanp force-pushed the add-check-events branch from b5849a5 to 66c53b9 Compare May 17, 2025 15:25
@marijanp marijanp marked this pull request as ready for review May 17, 2025 15:25
@marijanp marijanp force-pushed the add-check-events branch 3 times, most recently from 2410faf to 9d2a9ac Compare May 22, 2025 14:44
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

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_">
Copy link
Member

Choose a reason for hiding this comment

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

The documentation says this type also contain an (optional?) organization

check_run: check_run;
repository: repository;
sender: user;
installation: app_installation_event;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be optional?

] <json open_enum>

type check_run_event = {
action: check_run_action;
Copy link
Member

Choose a reason for hiding this comment

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

with the requested_action action, this type also has an (optional?) requested_action field

Copy link
Member

Choose a reason for hiding this comment

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

the action might also be optional but i'm not sure

repository: repository;
sender: user;
installation: app_installation_event;
} <ocaml field_prefix="check_suite_event_">
Copy link
Member

Choose a reason for hiding this comment

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

same for this one, an organization field might be there

Copy link
Member

Choose a reason for hiding this comment

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

The optional enterprise field is also available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants