-
-
Notifications
You must be signed in to change notification settings - Fork 777
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 add-label.js
to use label-directory.json
#7538
Refactor add-label.js
to use label-directory.json
#7538
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.
|
Hey @tamara-snyder and @codyyjxn Before you get too far reviewing this issue, I want to provide instructions for reviewing the PR, hopefully in the next day or two. Thanks! |
Thanks I was going to reach out to you. I definitely need a bit more
information so I can fully understand the issue. Thanks!
…On Mon, Sep 30, 2024 at 8:56 PM Will Gillis ***@***.***> wrote:
Hey @tamara-snyder <https://github.com/tamara-snyder> and @codyyjxn
<https://github.com/codyyjxn> Before you get too far reviewing this
issue, I want to provide instructions for reviewing the PR, hopefully in
the next day or two. Thanks!
—
Reply to this email directly, view it on GitHub
<#7538 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BGV5I3BZLZGU4BGNPY24KW3ZZIMNRAVCNFSM6AAAAABPBG6BB6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBUG4ZTENZUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi @t-will-gillis, I hope you are well. Is it okay if I review this PR too? I can get you a review by EOD Wednesday, 10/2. |
Hey @mrodz Yes, definitely! Thank you |
Hey @tamara-snyder, @codyyjxn, and @mrodz: I believe this is ready for review now. I added review notes, and I had to make some last-minute edits because I omitted some labels. As I said in the review notes, let me know if you run into any problems that you can not figure out quickly- I might know what the problem is and could save you a lot of headaches. Thanks! |
changed note to `labelKeys` from `labelIds`
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 @t-will-gillis Great Job refactoring the labels.
- After reviewing this the way you refactored the
label-directory.js
it seems like that was a great approach of retrieving the label names. - Also the
add-label.js
is returning the correct labels that you added. You also map through them correctly. - The branch is correctly named the issue is linked.
The only thing I recommend is changing the original issue from "New issue approval" to "Working in progress"
Also thank you for the notes on reviewing the issue. It definitely helped me understand how to go about it.
Keep it up!
Sorry for the delay! Thanks @t-will-gillis for adding some notes. I'll take a look at it today and come to office hours or reach out if I get stuck. |
Hi again @t-will-gillis, I got my environment set up (I think), but I haven't had time to test the issue yet. I'm travelling for the next few days, so I'm going to unassign myself from reviewing this just so it doesn't get held up. If it still needs a review when I return, I'll come back and look at it more. Thanks! |
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 @t-will-gillis! This pull request looks good—your changes look sound, and the updated comments reflect your migration from hardcoded labels to runtime assignment.
The only thing I will note is that it is not super intuitive to have labelRetrieveNames
be a variadic function if it always expects a single label parameter and does not do bounds checking for zero labels passed to labelKeys
. But this could be intended behavior and open the door for future implementations—I'd like to hear your thoughts.
Testing looks good! Thanks for including your own test run and steps for others to follow suit.
Please let me know if there is anything else you need from me. I apologize for the delay—work and school have been very consuming. I hope you're having a wonderful day :)
Hi @codyyjxn thank you for your review and comments. I corrected the issue status as you noted |
Refactored non-variatic funct, renamed funct
fix comments for singular use
Hey @mrodz Thank you for your review comments. Argh, you are correct about the variadic function- I overlooked this. Since Thank you for pointing this out! I have edited the PR to remove the variadic, renamed the function |
add comment first line, remove space
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.
@t-will-gillis
The changes look good!
The code runs on my environment.
Great job on this!
Fixes #7537
What changes did you make?
retrieve-label-directory
module and new variable definitions to uselabelId
- Consolidating to.map(retrieve-label-directory)
retrieve-label-directory.js
module to remove the for loopsince the labels are iterated using the spread syntax. Also, reformatted so thatlabelData
is generated with the first iteration only.UPDATE 10/14/24:
retrieve-label-directory.js
, removed the variadic parameter and refactored for a single parameterlabelRetrieveNames()
-->retrieveLabelName()
for consistency, and edited plurals to singularsWhy did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)
"Schedule Friday" log showing successful runNotes for Testing
When copying the Project Board, be sure to set yourself as the owner. You can rename the copy to "Project Board" or leave it.
After you make the copy, you will see an error message that says something to the effect that some workflows have failed to be created- you can safely ignore this message.
You will likely want to change the View to 'Board' to see that the status column names transferred correctly.
To match Hack for LA's Project Board's functionality, you may need to activate workflows by selecting the three dots in the upper right, then "Workflows". Set the "Workflows" as follows:
You will want to create the labels in your repo as explained in Tip 8.
add-label.js
, Please comment out Line 182minimizeComments(commentsToBeMinimized);