lostluck commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r898622963


##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+       "context"
+       "flag"
+       "fmt"
+       "log"
+
+       "github.com/apache/beam/sdks/v2/go/pkg/beam"
+       "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+       input_text = flag.String("input-text", "Default input text", "Input 
text to print.")
+)
+
+type Result = struct {
+       Pipeline    *beam.Pipeline
+       Scope       beam.Scope
+       PCollection beam.PCollection
+}
+
+func my_pipeline(input_text string) Result {
+       beam.Init()

Review Comment:
   This should be called in `main`, right after flag parsing. 



##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+       "context"
+       "flag"
+       "fmt"
+       "log"
+
+       "github.com/apache/beam/sdks/v2/go/pkg/beam"
+       "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+       input_text = flag.String("input-text", "Default input text", "Input 
text to print.")
+)
+
+type Result = struct {
+       Pipeline    *beam.Pipeline
+       Scope       beam.Scope
+       PCollection beam.PCollection
+}
+
+func my_pipeline(input_text string) Result {
+       beam.Init()
+
+       pipeline, scope := beam.NewPipelineWithRoot()
+
+       elements := beam.Create(scope, "Hello", "World!", input_text)
+       elements = beam.ParDo(scope, func(elem string, emit func(string)) { 
fmt.Println(elem); emit(elem) }, elements)

Review Comment:
   While an anonymous function *can* work in Beam Go, it will cause more 
trouble than it's worth. Move it to a standard definition, and use an init 
block to register the function.
   
   Otherwise, folks moving beyond the direct runner will have a bad time.
   
   ```
   func printAndEmit(elem string, emit(string)) { 
     fmt.Println(elem)
     emit(elem) 
   }
   
   func init() {
     // DoFns should be registered with Beam.
     beam.RegisterFunction(printAndEmit)
   }
   ```



##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+       "context"
+       "flag"
+       "fmt"
+       "log"
+
+       "github.com/apache/beam/sdks/v2/go/pkg/beam"
+       "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+       input_text = flag.String("input-text", "Default input text", "Input 
text to print.")
+)
+
+type Result = struct {

Review Comment:
   Secondly, while one *can* define a type for this, one shouldn't. Go would 
just use multiple returns. 
   
   In this case, I would recommend calling `pipeline, scope := 
beam.NewPipelineWithRoot()` in `main`, and passing in the scope variable. Then 
we simply return the PCollection.



##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+       "context"
+       "flag"
+       "fmt"
+       "log"
+
+       "github.com/apache/beam/sdks/v2/go/pkg/beam"
+       "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+       input_text = flag.String("input-text", "Default input text", "Input 
text to print.")
+)
+
+type Result = struct {

Review Comment:
   Type definitions don't use an `=`. Using an equal defines a type alias, 
which isn't what we want here.
   
   ```suggestion
   type Result struct {
   ```



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to