-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
removed unused variable in getDayString #7296
removed unused variable in getDayString #7296
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.
|
Availability: Weekdays 6:30-9PM, Weekends 9AM-5PM |
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.
Hi @pluto-bell, your code changes look good!
- Removed unused variable
- Documented changes
- Explained reasoning for changes
- Resolves CodeQL alert
- Correct
to
branch - Linked issue(s)
However, I believe you misread the instructions on the CodeQL issue:
Create an issue branch and proceed with the code update
This does not mean creating a new issue on top of the CodeQL issue and merging a PR to close two separate issues. Instead, make one branch associated with #7013 and develop your patch on there.
Can you please apply your changes to a branch associated with #7013, and close the extra issue (#7295)?
Here are some examples of merged PRs that fix CodeQL alerts:
- Remove unused import from post-comment.js #6824
- Removed Unused Variable in check-labels Action #6822
- fix: resolve CodeQL alert #6517
Re-request my review once you've got this done.
Thanks @mrodz! I did misread the instructions, and that makes way more sense. I've closed out issue #7295 and editing the description of this PR. Since the branch name is already linked to issue #7013 ("resolve-codeQL-alert#114-issue#7013") I didn't change anything there. If I'm misunderstanding what you're looking for, please let me know. |
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.
I see you closed the extra issue—nice! Thanks for cleaning it up. Everything looks good to me and I think you're in a good state to merge.
For future issues, you don't need to include the #
before any issue numbers. Also, you don't need to include the CodeQL alert number as prior PRs do not include it, and its CodeQL number can be obtained from the tracking issue.
Merge team, see my previous review. I am approving this issue as while the branch name does not explain the fixes, previously-merged CodeQL patches also do not relay changes in the branch name.
Hey @ihop-56, Could you please provide your estimated time for review and your availability? Thanks! |
Aug 23 9 PM ET
…On Thu, Aug 22, 2024, 3:24 PM J Pham ***@***.***> wrote:
Hey @ihop-56 <https://github.com/ihop-56>, Could you please provide your
estimated time for review and your availability? Thanks!
—
Reply to this email directly, view it on GitHub
<#7296 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BA5P262NMHHZKXXUHSV3UXTZSY3IDAVCNFSM6AAAAABMSWYXTSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBVGQ3TINJSGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
Hi @pluto-bell, I reviewed the changes, and everything looks great. The unused variable in the getDayString
function was correctly removed. There are no visual changes to the website, and the code is cleaner as a result. Nice job!
Fixes #7013
What changes did you make?
getDayString
in fileright-col-content.js
Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
No visual changes made