Fix RFC rendered linkifier. #1646
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `pull_request` event was not marking the issue as a pull_request. That means that some handlers were ignoring pull requests when they shouldn't have (like the rfc rendered handler). Additionally I removed the empty `PullRequestDetails` struct and replaced it with a bool instead. I don't foresee the `PullRequestDetails` ever getting fields added to it (the fields usually are part of `Issue`).
This changes the URL used in the RFC "Rendered" link to link directly to the author's branch instead of a specific commit. The problem with specific commits is that they need to be updated on every push. The existing code didn't handle that (it ignored if it already had a link), and I think SHA blob links are less desirable since they have a risk that reviewers will be looking at outdated content. This also drops the check for `Synchronize` events, since this isn't updating the rendered link on every push.
Mark-Simulacrum
approved these changes
Sep 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The RFC "Rendered" link generator wasn't working. The problem is that the
Pushevent wasn't marking the payload as a PR. When the link generator was looking for the list of files, thefilesmethod was returning nothing.I removed the empty
PullRequestDetailsstruct since I don't foresee that ever gaining any fields (the fields usually are part ofIssue). I think the bool makes the meaning a little clearer.Additionally I changed the format of the link URL. I prefer to have links directly to the author's branch so that it can reliably display the latest content. Linking to the SHA blob URL risks that the link gets out of date and reviewers may be looking at stale content. The drawback with the branch-based approach is that the link will die when the branch is deleted or reset. I think that is relatively good tradeoff, as I think the Rendered link should be updated when the RFC is closed. Perhaps that can be automated in the future.
Finally, I added some comments along the way to help understand what all these structs/fields/variants are doing.