Skip to content
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

Need for Speed: Minor improvements (#1755) #2235

Merged
merged 9 commits into from
Jun 18, 2022

Conversation

eklatzer
Copy link
Contributor

@eklatzer eklatzer commented Jun 7, 2022

Fixes #1755

Hey, I have done the following:

  • adjusted the instructions.md to clarify the struct-initialization and the usage of constructors
  • renamed variables for consistency

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

Dear eklatzer

Thank you for contributing to the Go track on Exercism! 💙
You will see some automated feedback below 🤖. It would be great if you can make sure your PR covers those points. This will save your reviewer some time and your change can be merged quicker.

  • 📜 The following files usually contain very similar content.

    • concepts/<concept>/about.md
    • concepts/<concept>/introduction.md
    • exercises/concept/<exercise>/.docs/introduction.md

    Please check whether the changes you made to one of these also need to be applied to the others.

  • ✍️ If your PR is not related to an existing issue (and is not self-explaining like a typo fix), please make sure the description explains why the change you made is necessary.

  • 🔤 If your PR fixes an easy to identify typo, if would be great if you could check for that typo in the whole repo. For example, if you found Unicdoe, use "replace all" in your editor (or command line magic) to fix it consistently.

Dear Reviewer/Maintainer

  • 📏 Make sure you set the appropriate x:size label for the PR. (This also works after merging, in case you forgot about it.)

  • 🔍 Don't be too nit-picky. If the PR is a clear improvement compared to the status quo, it should be approved as clear signal this is good to be merged even if the minor comments you might have are not addressed by the contributor. Further improvement ideas can be captured in issues (if important enough) and implemented via additional PRs.

  • 🤔 After reviewing the diff in the "Files changed" section, take a moment to think about whether there are changes missing from the diff. Does something need to be adjusted in other places so the code or content stays consistent?

Automated comment created by PR Commenter 🤖.

@eklatzer
Copy link
Contributor Author

eklatzer commented Jun 7, 2022

Hey, I adjusted the format of the outputs because I have seen #2202

@eklatzer eklatzer changed the title Need for Speed: Minor improvements Need for Speed: Minor improvements (#1755) Jun 7, 2022
@eklatzer
Copy link
Contributor Author

eklatzer commented Jun 7, 2022

The other changes should fix the TODO's from #1755

Copy link
Member

@andrerfcsantos andrerfcsantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just going to suggest a change in the order we talk about these concepts. I think we should talk about string literals first and then the New() or New***() functions to create structs. I would leave the explanation of the new builtin for last as it is not as relevant for the exercise .

Also note that the introduction.md for the exercise is often a copy of the introduction.md and about.md of the concept they teach, in this case structs, so the changes must also be reflected there. You can leave this for last, just mentioning so it's not forgotten.

@eklatzer
Copy link
Contributor Author

eklatzer commented Jun 7, 2022

I hope I have covered everything you wanted me to change

@eklatzer
Copy link
Contributor Author

eklatzer commented Jun 9, 2022

@andrerfcsantos I am a bit confused, have you already checked my changes, is something missing or what is the current state?

@andrerfcsantos
Copy link
Member

I did take a look at this and I have some changes in mind, but I want to think about them a little bit better before actually suggesting them. This is why this is taking longer to review, didn't have the chance yet to fully gather and formalize my thoughts - but it is not forgotten! :)

Copy link
Member

@andrerfcsantos andrerfcsantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left my review in about.md just to make things simpler, but please also apply the changes in the other files.

concepts/structs/about.md Outdated Show resolved Hide resolved
concepts/structs/about.md Show resolved Hide resolved
concepts/structs/about.md Show resolved Hide resolved
concepts/structs/about.md Show resolved Hide resolved
concepts/structs/about.md Outdated Show resolved Hide resolved
concepts/structs/about.md Outdated Show resolved Hide resolved
concepts/structs/about.md Outdated Show resolved Hide resolved
concepts/structs/about.md Outdated Show resolved Hide resolved
concepts/structs/about.md Show resolved Hide resolved
@andrerfcsantos andrerfcsantos added x:size/medium Medium amount of work status/awaiting-contributor This pull request is waiting on the contributor. labels Jun 10, 2022
@eklatzer
Copy link
Contributor Author

eklatzer commented Jun 10, 2022

@andrerfcsantos Thanks for the feedback, I hope I have addressed all of your messages.

Are more details needed in the introduction.md?

@andrerfcsantos andrerfcsantos added status/awaiting-maintainer This pull request is waiting on one or more maintainers. and removed status/awaiting-contributor This pull request is waiting on the contributor. labels Jun 17, 2022
@andrerfcsantos andrerfcsantos merged commit 71051bd into exercism:main Jun 18, 2022
@eklatzer eklatzer deleted the issue-1755 branch June 22, 2022 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/awaiting-maintainer This pull request is waiting on one or more maintainers. x:size/medium Medium amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need for Speed: Apply minor improvements
2 participants