Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upload: Fix review graph comment #177

Merged

Conversation

brian-kubisiak-skydio
Copy link

If there are two PRs in a relative chain where one PR's URL is a
prefix of another (e.g. when PR 24 is relative to PR 2), the review
graph comment will incorrectly modify the second URL by inserting '**'
at the start and in the middle of the URL. This is due to the naive
string replacement checking for any occurance of each URL.

Fix this by searching instead for the URL followed by the review title
and replacing them both at once instead of individually. This is also
slightly more efficient since there are fewer strings allocated.

Signed-off-by: Brian Kubisiak [email protected]

If there are two PRs in a relative chain where one PR's URL is a
prefix of another (e.g. when PR 24 is relative to PR 2), the review
graph comment will incorrectly modify the second URL by inserting '**'
at the start and in the middle of the URL. This is due to the naive
string replacement checking for any occurance of each URL.

Fix this by searching instead for the URL followed by the review title
and replacing them both at once instead of individually. This is also
slightly more efficient since there are fewer strings allocated.

Signed-off-by: Brian Kubisiak <[email protected]>
@brian-kubisiak-skydio
Copy link
Author

Reviews in this chain:
#177 upload: Fix review graph comment

@brian-kubisiak-skydio
Copy link
Author

# head base diff date summary
0 adbe2033 d29851a6 diff Apr 5 12:35 PM 1 file changed, 4 insertions(+), 3 deletions(-)

@jerry-skydio jerry-skydio merged commit 71ca5eb into Skydio:main Apr 5, 2024
5 checks passed
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