Skip to content

Conversation

@chromy
Copy link
Contributor

@chromy chromy commented Oct 30, 2025

Make functions extract_pr_{base,head}_sha_from_event match exactly except for
SHA they extract. Previously they have different APIs (Result<Option<String>> vs
Option<String>). We choose Option<String> since there seems little point forcing
users to handle the case of Github returning invalid json. Better options are:

  • Returning None
  • Crashing (panic!)

We switch both extract_pr_head_sha_from_event and
extract_pr_base_sha_from_event to panic.

@chromy chromy requested review from a team and szokeasaurusrex as code owners October 30, 2025 14:10
@chromy chromy force-pushed the chromy/2025-10-29-fix-base-part-1 branch from 710b5ee to 6f869fa Compare October 30, 2025 14:10
@chromy chromy changed the title ref(vcs): Make extract_pr_base_sha_from_event like extract_pr_head_sha_from_event ref(vcs): Make extract_pr_{base,head}_sha_from_event match Oct 30, 2025
@chromy chromy force-pushed the chromy/2025-10-29-fix-base-part-1 branch 3 times, most recently from a7e8a8d to 83c7be1 Compare October 30, 2025 14:21
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

lgtm, thank you for explaining the reasoning here.

One reason we might wish to crash instead of just logging a debug message: If GitHub ever changes the schema of the JSON event payload, in a way that this function would fail to extract the SHA, this would fail silently (debug messages are not logged at all by default, as the default log level is WARN). But, I'll leave it up to you to decide which way to go.

@chromy chromy force-pushed the chromy/2025-10-29-fix-base-part-1 branch from 83c7be1 to e12162c Compare October 30, 2025 16:11
cursor[bot]

This comment was marked as outdated.

@chromy chromy force-pushed the chromy/2025-10-29-fix-base-part-1 branch from e12162c to 254f4e6 Compare October 30, 2025 16:15
@chromy chromy force-pushed the chromy/2025-10-29-fix-base-part-1 branch from 254f4e6 to 354e2db Compare October 30, 2025 16:18
@chromy
Copy link
Contributor Author

chromy commented Oct 30, 2025

One reason we might wish to crash instead of just logging a debug message: If GitHub ever changes the schema of the JSON event payload, in a way that this function would fail to extract the SHA, this would fail silently (debug messages are not logged at all by default, as the default log level is WARN). But, I'll leave it up to you to decide which way to go.

sg! Is it enough to call panic! (I have updated the code to do that) or is there a fancier way to hard error?

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.

3 participants