-
-
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
Modify GitHub actions for label change 3109 #3839
Modify GitHub actions for label change 3109 #3839
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.
|
Linked below are two updates I provided in the comments for issue #3109:
|
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 @bzzz-coding – I am reviewing the changes you made, and it appears your edits match what the original issue was looking for. This is primarily the two files in post-labels-comment.js
and check-labels.js
Great job!
My one observation is about the text case for the labels. You matched what the original issue requested- i.e. to change Size:
to level:
so this isn’t a critique of your work. However, I am marking my review as ‘Request changes’ because I think that while we are already making the changes this is a great time to review the HfLA documents and code and make sure they are stylistically consistent. For example, our labels are sometimes all lowercase (role: back end/devOps
and size: 1pt
), all title case (Status: Updated
), or mixed (as Size: Medium
changes to level: Medium
) without there seeming to be a reason why the style should be one way and not another. (At least from what I can see. Maybe there is pattern here that I am not seeing…) What do others think?
As you mention, there are other places that reference the Size: Large
etc. For example, the original issue notes that the CONTRIBUTING.md
document also needs to be edited via #3110.
A big warning here is that #3110 is talking about changing Size:
to Complexity:
and not level:
(or Level:
) Probably needs to be edited?
This also leaves the wikis. I am not sure if the intent was for the wikis to be part of this issue or if they are a separate issue… merge team?
Thanks for your work! Looking good so far.
Hi @t-will-gillis, thanks for reviewing my PR and leaving the comment! I agree--it would be a good time to review the label styles and address the comment about changing Size to Complexity under the issue. I was also wondering if there is a general style guide regarding GHA and all the HfLA code regarding indentation, quotation marks, line breaks, naming variables, etc. |
Hi @bzzz-coding I do not know if HfLA has a style guide. It is possible that there is something in the wikis https://github.com/hackforla/website/wiki but I have not found that myself. I would like to know for myself also. This may be something to bring up in a meeting or by slacking the merge team. I also don't want to hold up this PR if we do not want to address the styling now. |
@bzzz-coding and @t-will-gillis, |
@blulady @t-will-gillis @hackforla/website-merge Thank you! I'll wait for further clarification/revision to this issue regarding the change from Size to level or Complexity and then make changes if needed. |
@hackforla/website-merge and in github-actions/trigger-issue/add-missing-labels-to-issues/post-labels-comment.js:
I only updated 'size' to 'Complexity' in this PR, and left ':' and 'missing' as they are. |
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 catch with the ':' in 'Complexity: missing'! Although I can see one side of an argument for keeping it, my opinion would be to ditch the colon and capitalize Missing to match:
const LABELS_OBJ = { 'Complexity Missing': 'Complexity', 'role missing': 'Role', 'Feature Missing': 'Feature' }
How do you feel about it?
I notice that Matt added this issue to the meeting agenda for January 29th. I am submitting this as a comment for now and will test this tomorrow to see whether removing the colon has any unexpected affects.
I tested with
'Complexity Missing' : 'Complexity'
- the test was OK/ no issues.
Hey @bzzz-coding We brought up your question about letter casing and the usage of |
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 @bzzz-coding - Awesome work and thanks for following through with it!
ETA: End of day Feb 1st |
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.
Amazing work on this one @bzzz-coding ! 👍
I appreciate your attention to detail noticing the label naming inconsistencies and would also like to commend the outstanding display of teamwork between you and @t-will-gillis on this PR
Fixes #3109
What changes did you make and why did you make them ?