[Jackenmen]

Fixes #547

The biggest change involves fetching PR comments - per https://github.com/orgs/community/discussions/213137 /repos/{owner}/{repo}/issues/{issue_number}/comments does not work on temporary private forks. Instead, I switched comment retrieval for pull requests to use a GraphQL query. The same data should be present in both old REST API and new GraphQL API implementation but the way it's structured into fields is not the same for some information. I used the implementation for discussion comments as a starting point so let me know, if there's any other field from IssueComment that you think is worth including.

[Iamrodos]

Thanks, good catch on temporary private forks. Supporting them shouldn't change how normal repositories are backed up unless there's a strong reason to, so a few things could do with adjusting.

The main one is the PR comment format. Switching every repository to GraphQL changes the saved structure of comment_regular_data, for example user/html_url/created_at become author/url/createdAt. Existing backups, and any tools reading them, rely on the current structure, and incremental runs would end up with both formats mixed together. GraphQL also needs a token, so PR comment backup would stop working for anyone running without one. Could you keep the REST call as it is and only fall back to the GraphQL query when it returns 404? That way only temporary forks get the new format and everything else stays the same.

On tests: 10 existing tests fail with KeyError: 'has_issues'. repository.get("has_issues", True) fixes that, and also covers GitHub Enterprise Server versions that may not return these fields. A test for the fork case would be great too, i.e. a 404 on the comments endpoint falling back to GraphQL.

The milestones/labels/releases 404 handling looks good.

[Jackenmen]

Thanks, good catch on temporary private forks. Supporting them shouldn't change how normal repositories are backed up unless there's a strong reason to, so a few things could do with adjusting.

The reason is not making PR comment data structure be dependent on whether it was made in a temporary private fork or not (or more specifically, whether the normal issue comments endpoint 404s or not).

The main one is the PR comment format. Switching every repository to GraphQL changes the saved structure of comment_regular_data, for example user/html_url/created_at become author/url/createdAt

If you think it's important to keep this structure, it would be possible to map the fields out to their REST API equivalents. I'm not sure whether that's an acceptable approach for you.

Existing backups, and any tools reading them, rely on the current structure, and incremental runs would end up with both formats mixed together.

Hmm, I guess incremental runs are going to be problematic, if the structure is now changed... If the above suggestion of mapping things out to old fields is acceptable then that would make things easier.

GraphQL also needs a token, so PR comment backup would stop working for anyone running without one.

Hmm, I was under the impression that the token is required for the tool to run anyway. Is that not the case?

Could you keep the REST call as it is and only fall back to the GraphQL query when it returns 404? That way only temporary forks get the new format and everything else stays the same.

I could but then the tools reading the backups could encounter both formats, making things less consistent (unless the above suggestion of mapping out fields is acceptable). I'm not sure that's such a good idea.

On tests: 10 existing tests fail with KeyError: 'has_issues'. repository.get("has_issues", True) fixes that, and also covers GitHub Enterprise Server versions that may not return these fields.

I simply forgot to update the test data to include the has_* fields that are in use now. has_issues and has_discussions are always present, including on GHES (has_discussions is available starting with GHES 3.3 which has been EOL since January 2023 - I doubt there's any need to support versions older than that). Documentation doesn't list has_pull_requests as a field at all, even for github.com so I swapped lookup for that with a get call just to be extra safe.

A test for the fork case would be great too

I can add that once we're clear on the other conversation points. Btw, you don't seem to be a maintainer or collaborator based on your badge - am I right in assuming that your feedback isn't necessarily going to be consistent with what the repo owner will want me to do in order to accept the contribution?

[Jackenmen]

incremental runs would end up with both formats mixed together.

One more thing worth noting is that the formats wouldn't be mixed within a single PR as comments are fetched from scratch every time a pull request is updated ("is due for repository checkpoint"). Between different PRs, however, the structures of comment_regular_data would differ. It's still not ideal to have things mixed at all but it is not as severe as mixing them within a single array/list would be.