-
-
Notifications
You must be signed in to change notification settings - Fork 843
Adding test instructions and links to CONTRIBUTING.md #7762 #8195
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
Adding test instructions and links to CONTRIBUTING.md #7762 #8195
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL: |
|
Availability: after 1pm (pacific) |
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.
This was a lot of work @aswutmaxcy. Thank you!
Things Done Well
- The pull request done with the correct branch.
- There's a linked issue. I understood it.
Suggestions/Questions (hope this is clear 🤞)
- The hyperlink on line 56 doesn't work. I can only guess how to fix this, so I won't say anything.
- This is a question for @santisecco. Why does the TOC say "ii. Testing your code changes" while the text says "ii. Testing your code edits"? Shouldn't both say "edits" or "changes" instead of being different? Is this why the hyperlink in the TOC doesn't work?
- Line 528 of your code renders with the asterisks, "**i. Creating your issue branch" instead of without them. I believe the asterisks are for formatting, so shouldn't you also add two asterisks at the end of your code on line 528? Please see line 526 of your code as an example because it uses two asterisks at the beginning and at end of the line of code.
- Line 554 says "edits" instead of "changes". It also renders in a smaller font size. Looking at #7762, I see "#### ii. Testing your code edits" with 4 octothorps (#), not the 5 you have in your code. Please delete one octothorp from line 554 and replace "edits" with "changes" in your code, or replace "changes" with "edits" in the TOC.
|
Availablity: After 5pm PST |
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 @aswutmaxcy great job with this! Here are some comments and observations:
- Please remove the note at the top of the PR beginning "Please note: You must be a member of the HFLA ..."
- Under "What changes did you make?" and "Why did you make the changes?" remove the empty formatting lines
- For "Why did you make the changes?", the response should be describing the reason for the change which should be in the "Overview" of the original issue. For this PR it should be something simple like "We need to remind developers to test their code changes."
- For the "CodeQL Alerts", please check the appropriate box (i.e. "I have checked this PR for CodeQL alerts and none were found.")
- For the "Screenshots of Proposed Changes..." thanks for deleting the default image info. Please add something like "- No visual changes to website"
- Current line 560: please capitalize 'GitHub' and 'Actions', that is:
[HfLA GitHub Actions](https://github.com/hackforla/website/wiki/HfLA-GitHub-Actions)<br>
@kdaca19xx had some questions/ comments (& now I know what 'octothorpe' means lol). We (= me) missed some of this previously, sorry about that.
-
Note that the following differs from @kdaca19xx 's note:
- The current line 528 should be 5 x '#' to match the formatting for the list items in rest of the document (such as lines 433-453).
- And there needs to be '**' at the end.
-
Therefore regarding the broken link on line 56, yes the text on current line 554 would need to match line 56- so
##### **ii. Testing your code changes** -
For consistency (and admittedly we are not always consistent), on the current line 564, please reference the label using backticks:
- Make sure to comment on your issue if you aren't sure how to test something, and then to put it in the **Questions / In Review column** and add the `ready for dev lead` label. Paste a link to your comments in the #hfla-site Slack channel asking peers for help. -
On line 558, please add the word "to" in the part of the line "...We don't want to debug or find solutions..."
Thanks for working on this issue!
xnealcarson
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 @aswutmaxcy ! Thanks for taking on this large issue!
Things Done Well
- The PR and issue were made on the appropriate branches
- There is a clear linked issue
- changes made were viewable
Suggested Changes
I don't have any additional changes to suggest besides the ones brought up by @kdaca19xx and @t-will-gillis .
Again, great job working on this issue!
|
@kdaca19xx Thank you so much for going through this with a fine tooth comb and finding the mistakes, I totally blanked on these! I just made all the changes you suggested and @t-will-gillis I also implemented the changes you suggested as well. Thank you both for taking the time to find these mistakes. |
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.
Hi @aswutmaxcy I see that you made changes to the description- thank you! However, I am not seeing any changes to the code per the previous reviews. It may be that you did not commit the updates?
Please check on the code updates. As a reminder: when you are ready, please remember to select the 'chasing-arrows' next to each reviewer's name to "Re-request review"
Thanks!
|
@t-will-gillis whoops! I'm a dumb, I just pushed the changes, it's up now |
New ETA: 6/26 (Thx for your changes and patience @aswutmaxcy !) |
kdaca19xx
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.
Thx for making the request changes, aswutmaxcy! I'd like to approve your PR.
xnealcarson
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.
Everything looks great @aswutmaxcy ! Thanks again for your hard work on this issue.
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 @aswutmaxcy Thanks for making the changes- just a couple of things:
-
Current line 528: please use (5) X '#', should be:
##### **i. Creating your issue branch** -
Current line 564: please move the backtick:
`ready for dev lead label`should be:
`ready for dev lead` labelAlmost there!
|
@t-will-gillis thank you for the catch! I just pushed 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 @aswutmaxcy Close! However, on line 564 the backticks need to be:
`ready for dev lead` label
|
@t-will-gillis If I messed it up again this time, I give you permission to burn my computer |
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.
Thank you, @aswutmaxcy !
|
PR has two previous reviews and Approvals, OK to merge. |
Fixes #7762
What changes did you make?
Why did you make the changes (we will use this info to test)?
CodeQL Alerts
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)