[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas

[GitBox <gi...@apache.org>]

henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r418443043



##########
File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go
##########
@@ -18,8 +18,9 @@ package task
 import "github.com/apache/beam/sdks/go/pkg/beam"
 
 func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
-	processFn := func(element int) int {
-		return element * 10
-	}
-	return beam.ParDo(s, processFn, input)
+	return beam.ParDo(s, multiplyBy10Fn, input)
 }
+
+func multiplyBy10Fn(element int) int {
+	return element * 10

Review comment:
       The "* 10" is not covered by the answer placeholder. Is it intentional?
   
   ![image](https://user-images.githubusercontent.com/5459430/80789287-24764800-8bbe-11ea-9594-d279d633e9e7.png)
   

##########
File path: learning/katas/go/Core Transforms/Map/lesson-info.yaml
##########
@@ -20,5 +20,4 @@
 content:
 - ParDo
 - ParDo OneToMany
-- MapElements
-- FlatMapElements
+- ParDo struct

Review comment:
       Can maybe slightly rename to "ParDo Struct"?

##########
File path: learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go
##########
@@ -18,10 +18,7 @@ package task
 import "github.com/apache/beam/sdks/go/pkg/beam"
 
 func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
-	processFn := &multiplyByFn{
-		Factor: 5,
-	}
-	return beam.ParDo(s, processFn, input)
+	return beam.ParDo(s, &multiplyByFn{Factor: 5}, input)

Review comment:
       How about having the whole "&multiplyByFn{...}" be part of the answer placeholder?
   ![image](https://user-images.githubusercontent.com/5459430/80789407-8171fe00-8bbe-11ea-8ec6-e8ed8989ec2e.png)
   

##########
File path: learning/katas/go/Core Transforms/Map/ParDo struct/task.md
##########
@@ -16,10 +16,10 @@
     specific language governing permissions and limitations
     under the License.
 -->
-# Mapping Elements using structs
+# Using a struct as a DoFn

Review comment:
       Can we have the ParDo mentioned, e.g. "ParDo - using a struct as a DoFn"?

##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go
##########
@@ -21,10 +21,10 @@ import (
 )
 
 func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
-	return beam.ParDo(s, processFn, input)
+	return beam.ParDo(s, tokenizeFn, input)
 }
 
-func processFn(input string, emit func(out string)) {
+func tokenizeFn(input string, emit func(out string)) {

Review comment:
       The answer placeholder(s) seems weird here. Maybe it requires a fix?
   Or alternatively, we can use a similar answer placeholder as in the "ParDo" task, i.e. having just empty "fund ApplyTransforms"?
   ![image](https://user-images.githubusercontent.com/5459430/80789567-e88fb280-8bbe-11ea-9185-b87c867f1bee.png)
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org