-
-
Notifications
You must be signed in to change notification settings - Fork 843
Refactor minimizeIssueComments() to Avoid Limit #8155
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
Refactor minimizeIssueComments() to Avoid Limit #8155
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes. |
t-will-gillis
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.
Hey @katiejnete Fantastic job-
- The 'from' and 'to' branches are good
- Linked to the original issue
- The responses for "What changes did you make?" and "Why did you make the changes?" are great. You might get away with saying less, but at the same time this is a really good explanation of the reasoning behind this issue.
- There are no CodeQL alerts
- There is a workflow log showing that the workflow runs successfully after the edits
I have a couple of minor requests:
- Although the issue (that I wrote, ha) says to place
after
const upperLimitCutoffTime = new Date(); upperLimitCutoffTime.setDate(upperLimitCutoffTime.getDate() - upperLimitDays);const fourteenDayCutoffTime, to follow the same pattern as the rest of this section could you move the above code so that it is afterfourteenDayCutoffTime.setDate(fourteenDayCutoffTime.getDate() - inactiveUpdatedByDays);instead? - Another minor request, could you add a blank line to separate Line 35
const upperLimitDays = 30; // Bot-generated...and Line 36const threeDayCutoffTime = new Date();? I think keeping a newline between these two sets looks cleaner...
Thanks for working on this, and fantastic job again!
|
Thank you @t-will-gillis ! I've made the changes you've requested. |
t-will-gillis
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.
Great- thanks @katiejnete!
Fixes #7562
What changes did you make?
minimizeIssueComments()to add an upper time limit when hiding bot-generated comments.Why did you make the changes (we will use this info to test)?
CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)