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

add more details and examples to the introduction #2171

Merged
merged 39 commits into from Apr 15, 2022
Merged

add more details and examples to the introduction #2171

merged 39 commits into from Apr 15, 2022

Conversation

ghost
Copy link

@ghost ghost commented Apr 10, 2022

Before this commit, the introduction of this exercise has been a bit confusing for learners due to its incompleteness.

Before this commit, the introduction of this exercise has been a bit confusing for learners due to its incompleteness.
@github-actions
Copy link
Contributor

Dear kekimaker

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 🤖.

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.

Thanks a lot for opening this issue. You bring up some good and necessary points for improvement here. I left some inline comments.

Other than those inline comments, I want to suggest further changes overall.

In exercism, for a concept exercise we have 3 files that must have similar contents:

  • concepts/<concept>/introduction.md - This should contain an introduction to the concept with the necessary information for a student to solve exercises that use these concepts. It's what the student first sees when going for the concept page for the first time.
  • concepts/<concept>/about.md - Should contain the same contents of the previous file, but can contain more advanced information that can be useful, but it's not strictly necessary to solve the exercises. It's what the student sees after "mastering" the concept, i.e solving the exercise about this concept.
  • exercises/concept/<exercise>/.docs/introduction.md - Usually contains the same info as the first file (concepts/<concept>/introduction.md) but can contain more specific information about the exercise.

For more information about the structure of a concept exercise, check this page on our docs.

This means that when we change one file, we must check if the others must also be changed so they are in sync. This applies here, where you only changed exercises/concept/<exercise>/.docs/introduction.md - but the other files must also be reviewed. So I'd like this opportunity and take the good points you raised to improve those files too. Here's what I think we should include in those files in general terms, feel free to disagree:

  • concepts/strings/introduction.md - We can include the necessary info for every exercise about strings. Talking about the most used special characters here can be a good idea and having your examples for string functions is also a good idea.
  • concepts/strings/about.md - We should include the same information as the previous file, but we can add more information, like the complete list of special characters (similar to what we have now on this file)
  • exercises/concept/welcome/.docs/introduction.md - Put the same contents as concepts/strings/introduction.md here as a start, but you can include more examples of string functions that will be necessary for the exercise here. If there's something else about strings that you feel is needed for this exercise in particular, also include examples about those here.

Since this will require you to put some effort into the concept/exercise, feel free to add as a contributor for the exercise and for the concept after making these changes. This will award you some extra rep for the additional effort :) For the concept, add your GitHub handle here:

"contributors": []

For the exercise, add it here:

@@ -11,19 +11,30 @@ firstName := "Jane"
Strings can be concatenated via the `+` operator:

```go
fullName := "Jane" + " " + "Austen"
fullName := "Jane" + " " + "Austen" // Output: "Jane Austen"
Copy link
Member

Choose a reason for hiding this comment

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

Recently we've been discussing how using "Output" on statements that are not print statements can be confusing. We are looking to remove these instances. Here you can keep using variables but remove the "Output" bit from the comments. Alternatively, you should keep the "Output" in the comments, and use print statements for the strings. Because of the examples with special characters, maybe using a print statement would be the best solution here.

For more context about this change, see our previous discussion in #2156

Comment on lines 30 to 38
strings.ToLower("MaKEmeLoweRCase") // Output: "makemelowercase"

strings.ToUpper("MaKemeUpPercaSe") // Output: "MAKEMEUPPERCASE"

strings.Repeat("Go", 3) // Output: "GoGoGo"

strings.ReplaceAll("your cat is playing with your pillow", "your", "my") // Output: "my cat is playing with my pillow

strings.TrimSpace(" \t\n Hello, Gophers \n\t\r\n") // Output: "Hello, Gophers"
Copy link
Member

Choose a reason for hiding this comment

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

I think these examples are helpful! Good point.

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Almost ready, but I need to review the changes for any possible mistakes.

@ghost ghost requested a review from andrerfcsantos April 11, 2022 03:16
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I made the required changes. Kindly please review them @andrerfcsantos

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.

Suggested more changes, feel free to discuss any of them.

To avoid repeating suggestions, I only suggested them in one place, but some need to be applied everywhere they appear.

concepts/strings/about.md Outdated Show resolved Hide resolved
concepts/strings/about.md Outdated Show resolved Hide resolved
concepts/strings/about.md Outdated Show resolved Hide resolved
concepts/strings/about.md Outdated Show resolved Hide resolved
concepts/strings/about.md Outdated Show resolved Hide resolved
concepts/strings/about.md Outdated Show resolved Hide resolved
concepts/strings/about.md Outdated Show resolved Hide resolved
//Joe
//William
//Jack
//Averell
```

Raw string literals use backticks (\`) as their delimiter instead of double quotes and are interpreted literally, meaning that there is no need to escape characters or newlines.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Raw string literals use backticks (\`) as their delimiter instead of double quotes and are interpreted literally, meaning that there is no need to escape characters or newlines.
## Raw string literals
Raw string literals use backticks (\`) as their delimiter instead of double quotes and all characters in it are interpreted literally, meaning that there is no need to escape characters or newlines.

Apart from the example given here, I think it would be useful to include another one where special characters like \nare used to show the difference. We do something similar in the regular expressions concept where a regular double-quoted string literal is compared to a raw string literal:

"\t\n" // regular string literal with 2 characters: a tab and a newline
`\t\n`// raw string literal with 4 characters: two backslashes, a 't', and an 'n'

You can think of a better example, but feel free to use the same example as the one in that concept if you want. Since this is relevant in both places, I don't mind a little bit of repetition.

concepts/strings/introduction.md Outdated Show resolved Hide resolved
concepts/strings/introduction.md Outdated Show resolved Hide resolved
kekimaker and others added 9 commits April 12, 2022 10:22
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
Co-authored-by: André Santos <[email protected]>
@ghost ghost requested a review from andrerfcsantos April 13, 2022 07:15
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.

Thanks for this, let's get it merged :)

There were some updates in the discussion in #2156 since my last suggestion. In that issue we agreed on an way of presenting the output of solutions that contradicts my previous suggestions.

After talking with @junedev we also think that giving fewer examples on the string package can be good to help the students explore the documentation.

Since both of these changes are minor and contradict some of my previous suggestions, to not make you do more busy work on this PR, I applied them for you in a new commit.

@andrerfcsantos andrerfcsantos added the x:size/small Small amount of work label Apr 15, 2022
@andrerfcsantos andrerfcsantos merged commit 4913ba8 into exercism:main Apr 15, 2022
@ghost ghost deleted the patch-2 branch April 15, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/small Small amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant