Issue.is_pull_request: Bool vs richer pull_request type

[SeanTAllen]

The GitHub Issues API (GET /repos/{owner}/{repo}/issues) returns both issues and pull requests. PRs are distinguishable by the presence of a pull_request object in the JSON response:

{
  "number": 42,
  "title": "Fix the thing",
  "pull_request": {
    "url": "https://api.github.com/repos/owner/repo/pulls/42",
    "html_url": "https://github.com/owner/repo/pull/42",
    "diff_url": "https://github.com/owner/repo/pull/42.diff",
    "patch_url": "https://github.com/owner/repo/pull/42.patch",
    "merged_at": "2024-01-15T10:30:00Z"
  }
}

For non-PR issues, this field is absent entirely.

The current implementation (PR #54) adds is_pull_request: Bool to the Issue model. This discussion explores whether that's the right long-term representation.

Current approach: is_pull_request: Bool

The IssueJsonConverter checks obj.contains("pull_request") and sets the boolean.

How to work around the boolean when you need more

If a caller needs PR details (URLs, merge status), they can use the issue number to fetch the full pull request:

if issue.is_pull_request then
  repo.get_pull_request(issue.number).next[None]({
    (r: PullRequestOrError) =>
      match r
      | let pr: PullRequest =>
        // now you have full PR details
      end
  })
end

This requires an additional API call per PR, but gives access to the complete PullRequest model.

Pros

• Simple, no new types needed
• Covers the primary use case (filtering issues from PRs in list results)
• Matches the pony-sync-helper's actual need: it only checks presence, never reads the URLs
• The PullRequest model already exists for when full details are needed

Cons

• Loses data from the API response: merged_at, url, html_url, diff_url, patch_url
• merged_at is particularly notable: it's the only way to check merge status without a separate API call
• Users who want to check "is this PR merged?" must make an extra GetPullRequest call

Alternative: pull_request: (PullRequestLinks | None)

class val PullRequestLinks
  let url: String
  let html_url: String
  let diff_url: String
  let patch_url: String
  let merged_at: (String | None)

class val Issue
  // ...
  let pull_request: (PullRequestLinks | None)

Checking if an issue is a PR

match issue.pull_request
| let pr_links: PullRequestLinks =>
  // it's a PR; pr_links has URLs and merged_at
| None =>
  // it's a real issue
end

Pros

• Preserves all data from the API response
• merged_at available without extra API call
• Type-safe: matching on (PullRequestLinks | None) naturally handles the "is it a PR?" question
• No data loss

Cons

• New type (PullRequestLinks) and its converter
• More complex IssueJsonConverter
• The is_pull_request boolean check becomes a match expression (slightly more verbose at the call site)

Hybrid: both boolean and full type

Could add both is_pull_request: Bool (convenience) and pull_request: (PullRequestLinks | None) (full data). But this violates DRY since is_pull_request is always equivalent to pull_request isnt None.

Questions

1. Is the extra API call for merged_at an acceptable tradeoff, or is that field important enough to preserve?
2. Is match issue.pull_request | let _: PullRequestLinks => ... too verbose compared to if issue.is_pull_request then?
3. Are there other use cases beyond pony-sync-helper that would benefit from having the full pull_request data?