Add channels concept #2324

RezaSi · 2022-07-14T18:42:28Z

for this #2182

github-actions · 2022-07-14T18:42:43Z

Dear RezaSi

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 you changed the function signature or the function comment in the exemplar file or the stub file (<exercise>.go), make sure the change is applied to both files.
🔗 If your PR fully fixes an issue, please include the text Fixes #issue_no in any line of the PR description. This will make the issue be automatically be closed when the PR is merged. If your PR is related to an existing issue but does not fix it completely, please link the issue anywhere in the description of the PR with #issue_no. You can read more about this in Github: Linking a pull request to an issue
✍️ 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 🤖.}

junedev

The content you present here seems to be copied from https://www.golangbootcamp.com/book/concurrency. The book does not have any license file so we cannot know if the author allows copying without attribution. GoBootcamp/book#48 This is too risky for us.

The content needs to written in your own words, copied from places that explicitly allow copying without attribution (e.g. via an MIT license) or the sentences that are copied need to be quoted and the source needs to be added as a footnote (this is legally only allowed for short sections if not explicitly permitted otherwise).

I only looked at the first part so far. I don't know for how much of the total content this applies to. It's easier for you to know. In any case, please rework all the copied content so it follows the rules above. If you have any questions, let me know.

junedev · 2022-07-16T16:10:30Z

Did you invent the exercise from scratch? If yes, I could start reviewing that part.

RezaSi · 2022-07-16T16:44:37Z

The content you present here seems to be copied from https://www.golangbootcamp.com/book/concurrency. The book does not have any license file so we cannot know if the author allows copying without attribution. GoBootcamp/book#48 This is too risky for us.

The content needs to written in your own words, copied from places that explicitly allow copying without attribution (e.g. via an MIT license) or the sentences that are copied need to be quoted and the source needs to be added as a footnote (this is legally only allowed for short sections if not explicitly permitted otherwise).

I only looked at the first part so far. I don't know for how much of the total content this applies to. It's easier for you to know. In any case, please rework all the copied content so it follows the rules above. If you have any questions, let me know.

Yes, some part of the intro and about is copied from this site and golang tut, I don't know this, but it's ok, I will try to write it again with my words!

RezaSi · 2022-07-16T16:47:06Z

Did you invent the exercise from scratch? If yes, I could start reviewing that part.

Yes, it's not copied from somewhere or another exercise, tnx for the review.

junedev

I had a look at the about.md file. I will check out the exercise later on.

junedev · 2022-07-19T15:12:00Z

concepts/channels/.meta/config.json

+{
+  "blurb": "Channels are the pipes that connect concurrent goroutines.",
+  "authors": ["RezaSi"],
+  "contributors": ["RezaSi"]


You only need to add yourself as author. A contributor is someone who makes small adjustments later on (or the reviewer if you think they had substantial impact on the result).

junedev · 2022-07-19T15:29:38Z

concepts/channels/about.md

@@ -0,0 +1,115 @@
+# About


Could you please ensure some general points are taken care of throughout the content:

One line per sentence. (see https://exercism.org/docs/building/markdown/markdown#h-one-sentence-per-line)

"golang" should be "Go". (see https://go.dev/doc/faq#go_or_golang)

Make sure sentences start with a capital letter.

If your text contains some inline code (even if it is just one work like "make"), that bit needs to be wrapped in backticks for better readability, see https://exercism.org/docs/building/markdown/markdown#h-code.

If you can, please double check the grammar. If you are not a native speaker and have no one to help you with this, I will do the corrections later on before I merge the code.

junedev · 2022-07-19T15:44:09Z

concepts/channels/about.md

@@ -0,0 +1,115 @@
+# About
+
+As you know, goroutines are a way to execute concurrent code in golang.


Remove "as you know". It might be a long time since the student did that exercise and they might have forgotten.

On the word "goroutines" add a link to some document that explains goroutines. This helps students that forgot about them and it helps everyone as long as we don't have the goroutines concept on Exercism yet. Potential link: https://go.dev/tour/concurrency/1

junedev · 2022-07-19T15:45:43Z

concepts/channels/about.md

+value := <-ch  // Receive from ch, and assign value to value variable.
+```
+
+Create a new channel with make(chan val-type). Channels are typed by the values they convey.


I would recommend to first show how to create a channel and then how to send/receive data. That makes it easier to understand the comments you have above.

concepts/channels/about.md

junedev · 2022-07-19T15:51:12Z

concepts/channels/about.md

+	channel := make(chan bool)
+	go exist(slice[:len(slice)/2], lookForValue, channel)
+	go exist(slice[len(slice)/2:], lookForValue, channel)
+	x, y := <-channel, <-channel // receive from channel


Suggested change

x, y := <-channel, <-channel // receive from channel

x, y := <-channel, <-channel // receive 2 values from channel

junedev · 2022-07-19T15:52:22Z

concepts/channels/about.md

+	slice := []int{1, 7, 8, -9, 12, -1, 13, -7}
+	lookForValue := 8
+
+	channel := make(chan bool)


results or resultCh might be a better name here

junedev · 2022-07-19T15:58:03Z

concepts/channels/about.md

+### Notes: 
+- Only the sender should close a channel, never the receiver. Sending on a closed channel will cause a panic.
+
+- Channels aren't like files; you don't usually need to close them. Closing is only necessary when the receiver must be told there are no more values coming, such as to terminate a range loop.


This seems to be a quote from Tour of Go and should be marked as such. You can quote with ">" in markdown and you can use a footnote to state the source. Their license only allows to use the material if you set the same license for the derived content which we don't have at the moment.

As discussed before, this applies to all the content in the PR. Please either write things yourself or quote with source. I am sorry but if your don't stick to those legal rules, I will be forced to reject the PR.

The rules also apply to code examples btw.

RezaSi · 2022-07-21T09:08:18Z

@junedev Tnx for all your advice, I think resolved all, and repair some other parts.
Yes, I'm not a native speaker, if you can help with grammar, it's very welcome.

RezaSi · 2022-08-07T11:07:31Z

@junedev Any update?

junedev · 2022-08-12T11:26:33Z

@RezaSi Sorry for the wait. I had to discuss with the Exercism leadership how to proceed with this PR. The problem is that the concept content still contains some sentences that are copied without attribution and some others were there were only slight changes made to the original content. This happened after I mentioned the issue twice already. It is too risky for us to merge content like this and it takes too much time for me to check and discuss every sentence. I will still consider the exercise part (without introduction.md) for merging but before I merge, I will remove the concept content from the PR and set the exercise status to "work in progress". Then there will be a follow up issue about writing the concept from scratch and once someone else worked on this, the exercise can go live.

Hopefully, I can find some time to review the exercise on the weekend but I can't promise. I'm sorry this all takes so long but keep in mind that all track maintainers on Exercism are volunteers that do this work in their free time.

junedev

As mentioned before, I reviewed the exercise. I like the general idea about fetching data concurrently. I did not review the test file yet as it might change as you make some other adjustments. I will take a closer look once the other comments are addressed.

junedev · 2022-08-14T12:16:51Z

config.json

+            "for-loops",
+            "range-iteration"
+        ],
+        "status": "beta"


As discussed, please change the status to "wip" for now.

junedev · 2022-08-17T18:07:24Z

exercises/concept/cache/.docs/instructions.md

@@ -0,0 +1,33 @@
+# Instructions
+
+In this exercise you'll be writing code to handle multilayer cache with Go channels.


When you say "multilayer cache", I imagine something like what is usually called "multi-level cache" which would mean you would check different cache levels in a fixed order and the deeper in the level you go the longer it takes to retrieve the data. The issue is that in such a scenario, you would always make the request sequentially and only proceed to the next level/layer when no result was returned. The word "layer" puts emphasis on this, layers are usually passed/pealed one by one. It might be less confusing to only say there are multiple caches available or talk about a cluster of caches (although the later also has its existing meaning which is different from the what you want to do in the exercise).

In any case, add one or two more sentences explaining what you mean by whatever term you decide to use to avoid misunderstandings.

junedev · 2022-08-17T18:10:45Z

exercises/concept/cache/.docs/instructions.md

+type Cache struct {
+	ResponseTime time.Duration
+	Data         map[string]string
+}


I was wondering why this is presented in the instructions. It feels like this an implementation detail of the tests and it would be enough if the instructions would only state what the cache interface looks like (has a Get and Set method). Then the tests can provide whatever implementation for that interface. The internals of the cache should not matter for solving the exercise. You only need to say something like "some caches are faster to respond than others".

Showing this cache structs might make the student think they can solve the exercise by simply checking the value of "ResponseTime".

junedev · 2022-08-17T18:18:10Z

exercises/concept/cache/.docs/instructions.md

+
+
+```go
+GetValueFromFastestLayer(cacheLayers []Cache, key string) (string, error)


The code here should not present the function signature, instead if should show an example of how the function is called/used (and a potential result). You don't have to include the definition of the cache structs in here though.

Same for the other code block below.

junedev · 2022-08-17T18:22:02Z

exercises/concept/cache/.docs/instructions.md

+
+## 2. Set value to all layers
+The key and value are given to you as `key` and `value` parameters.
+You have to set the value to all `cacheLayers` as fast as possible, you have maximum `max(cacheLayers.ResponseTime)` to set the value to all layers.


I'm struggling with this sentence as using the channels will have some overhead so you won't exactly hit the max response time. Maybe you can rephrase it a bit to say something like "Try to get as close as possible to the response time of the slowest cache."

junedev · 2022-08-17T18:59:59Z

exercises/concept/cache/.meta/example.go

+	ch := make(chan string, len(cacheLayers))
+	for _, cache := range cacheLayers {
+		go func(cache Cache) {
+			value, _ := cache.Get(key)


The Get function should either not have an error return value or the error should be checked an if it is not nil, no value or an empty string should be send over the channel. It is good practice not to trust the result value in case the error is not nil.

junedev · 2022-08-17T19:20:33Z

exercises/concept/cache/.meta/example.go

+func SetValueToAllLayers(cacheLayers []Cache, key string, value string) error {
+	ch := make(chan error, len(cacheLayers))
+	for _, cache := range cacheLayers {
+		go func(cache Cache) {
+			err := cache.Set(key, value)
+			ch <- err
+		}(cache)
+	}
+
+	for range cacheLayers {
+		<-ch
+	}
+
+	return nil
+}


A couple of thoughts about this task (and the solution):

Currently, the function always returns nil so it could also just not have a return value at all.

The instructions should clarify what happens in case Set returns an error.

Overall, the required solution here is very similar to the one of the first task. Usually we try to bring up some new aspect in each task. Have another look at the learning objectives, maybe the task can be adjusted/re-written to cover something different. For example, currently your tasks don't cover ranging over a channel at all.

junedev · 2022-08-17T19:21:38Z

exercises/concept/cache/cache.go

@@ -0,0 +1,9 @@
+package cache
+
+func GetValueFromFastestLayer(cacheLayers []Cache, key string) (value string, err error) {


Please remove the names from the return values. Those should only be used when really needed as they can lead to mistakes easily.

junedev · 2022-08-17T19:23:29Z

exercises/concept/cache/cache_struct.go

@@ -0,0 +1,31 @@
+package cache


If you follow the suggestion mentioned above with the interface, the cache implementation can be part of the standard test file. If you want to keep it as a separate file, you can list it in the config under the "editor" key so it shows up when solving the exercise but cannot be edited on the website.

junedev · 2022-08-17T19:24:12Z

exercises/concept/cache/cache_struct.go

@@ -0,0 +1,31 @@
+package cache


Instead of "cache_struct.go", "types.go" is a more common name for a file like this.

github-actions · 2022-09-17T04:14:08Z

Hello contributor and thank you for your contribution!
This pull request has been automatically marked as stale because it has not had recent activity. Please update the PR when you are able.
If this pull request is waiting on the maintainer, please apply the status/awaiting-maintainer label (and remove the status/awaiting-contributor label).
If no action is taken in the next week, this pull request will be automatically closed.

junedev · 2022-10-01T15:28:12Z

Closing this as it seems abandoned.

RezaSi force-pushed the channels branch from 0f5d1d9 to 3f1be62 Compare July 14, 2022 20:59

RezaSi mentioned this pull request Jul 16, 2022

Implement New Concept Exercise: Channels and Select #2182

Open

junedev requested changes Jul 16, 2022

View reviewed changes

RezaSi force-pushed the channels branch from 3f1be62 to 639b860 Compare July 16, 2022 19:19

RezaSi requested a review from junedev July 16, 2022 19:20

RezaSi force-pushed the channels branch from 639b860 to caab701 Compare July 17, 2022 06:39

junedev requested changes Jul 19, 2022

View reviewed changes

RezaSi force-pushed the channels branch from caab701 to 82c9511 Compare July 21, 2022 08:54

RezaSi requested a review from junedev July 21, 2022 09:09

RezaSi force-pushed the channels branch 6 times, most recently from 03a8465 to 4649624 Compare July 22, 2022 08:39

Add channels concept

891e190

RezaSi force-pushed the channels branch from 4649624 to 891e190 Compare July 22, 2022 12:59

junedev requested changes Aug 17, 2022

View reviewed changes

junedev added the status/awaiting-contributor This pull request is waiting on the contributor. label Aug 17, 2022

junedev linked an issue Aug 17, 2022 that may be closed by this pull request

Implement New Concept Exercise: Channels and Select #2182

Open

github-actions bot added action/stale and removed action/stale labels Sep 17, 2022

junedev closed this Oct 1, 2022

junedev added the abandoned The creator of this issue or PR does not respond anymore. label Oct 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add channels concept #2324

Add channels concept #2324

RezaSi commented Jul 14, 2022 •

edited

Loading

github-actions bot commented Jul 14, 2022

junedev left a comment

junedev commented Jul 16, 2022

RezaSi commented Jul 16, 2022

RezaSi commented Jul 16, 2022

junedev left a comment

junedev Jul 19, 2022

junedev Jul 19, 2022

junedev Jul 19, 2022

junedev Jul 19, 2022

junedev Jul 19, 2022

junedev Jul 19, 2022

junedev Jul 19, 2022

RezaSi commented Jul 21, 2022

RezaSi commented Aug 7, 2022 •

edited

Loading

junedev commented Aug 12, 2022

junedev left a comment

junedev Aug 14, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

junedev Aug 17, 2022

github-actions bot commented Sep 17, 2022

junedev commented Oct 1, 2022

		@@ -0,0 +1,115 @@
		# About

		As you know, goroutines are a way to execute concurrent code in golang.

	x, y := <-channel, <-channel // receive from channel
	x, y := <-channel, <-channel // receive 2 values from channel

		@@ -0,0 +1,33 @@
		# Instructions

		In this exercise you'll be writing code to handle multilayer cache with Go channels.



		```go
		GetValueFromFastestLayer(cacheLayers []Cache, key string) (string, error)

		@@ -0,0 +1,9 @@
		package cache

		func GetValueFromFastestLayer(cacheLayers []Cache, key string) (value string, err error) {

Add channels concept #2324

Add channels concept #2324

Conversation

RezaSi commented Jul 14, 2022 • edited Loading

github-actions bot commented Jul 14, 2022

Dear RezaSi

Dear Reviewer/Maintainer

junedev left a comment

Choose a reason for hiding this comment

junedev commented Jul 16, 2022

RezaSi commented Jul 16, 2022

RezaSi commented Jul 16, 2022

junedev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RezaSi commented Jul 21, 2022

RezaSi commented Aug 7, 2022 • edited Loading

junedev commented Aug 12, 2022

junedev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2022

junedev commented Oct 1, 2022

RezaSi commented Jul 14, 2022 •

edited

Loading

RezaSi commented Aug 7, 2022 •

edited

Loading