-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Project HashMap: Improve requirement clarity #29013
Conversation
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.
Nice work. Looks mostly good, just have one small nit and one small thing to discuss before I approve/merge. Let me know what you think.
Co-authored-by: Josh Smith <jmsmith1018@gmail.com>
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.
Nice work. Just one more small change to get the Markdown linter to pass, and then I'll be good to merge.
Another thing: the Ruby course has this exact same project in it just with example code written in the Ruby language. Would you be comfortable making a new PR that makes these updates there as well? You shouldn't have to know any Ruby to do this, and if you had any questions, I'd be glad to help.
Co-authored-by: Josh Smith <jmsmith1018@gmail.com>
@JoshDevHub thanks for the catch, I fixed the linter issue. I went ahead and made the changes to the Ruby lesson on the newest commit after the lint fix because I think it makes more sense to keep the changes synced in the same PR. If you'd rather I open a new PR, 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.
Alright, looking good. And it's fine to have the Ruby stuff in this PR as well.
Just a couple small changes related to differences in Ruby and JS, but after those are good to go, I'll be glad to merge.
Thank you for making these changes. I think they make the project a lot better 🥇
Co-authored-by: Josh Smith <jmsmith1018@gmail.com>
Co-authored-by: Josh Smith <jmsmith1018@gmail.com>
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.
Looks great. Thank you so much for putting in the work on this.
Because
This PR
Issue
Related to #27682
Additional Information
Discord username: @phosphorflux
Pull Request Requirements
location of change: brief description of change
format, e.g.Intro to HTML and CSS lesson: Fix link text
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section