-
Notifications
You must be signed in to change notification settings - Fork 161
Expire PR previews based on PR activity #4481
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
Conversation
Eric-Arellano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Feel free to push back, but I think this might be worth doing: the logic is fairly complex for get_pr_folders. Consider unit testing it. I'd recommend refactoring it so that the run_subprocess isn't part of the test, since that's non-deterministic.
Co-authored-by: Eric Arellano <[email protected]>
|
|
||
| parsed = json.loads(api_response, object_hook=parse_updated_at) | ||
| all_pr_numbers = (number for number in parsed) | ||
| all_pr_numbers = (pr["number"] for pr in parsed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing you suggested tests – this wasn't caught by the type checker because parsed is Unknown
Eric-Arellano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing tests!
Co-authored-by: Eric Arellano <[email protected]>
The current PR expiration logic looks at the time PR files were last modified. This PR switches the check to look at the time the PR was last updated. This could be a better metric in cases where the PR is undergoing a long review.