lostluck commented on code in PR #1: URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r900425256
########## .github/workflows/test.yaml: ########## @@ -0,0 +1,22 @@ +# Copyright 2022 Google LLC +# +# Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +# https://www.apache.org/licenses/LICENSE-2.0> or the MIT license +# <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your +# option. This file may not be copied, modified, or distributed +# except according to those terms. + +name: Test + +on: [push, pull_request] + +jobs: + tests: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v3 + with: + go-version: '>=1.18.0' Review Comment: Reduce maintenance for us and users, and have the version pulled from the go.mod file: https://github.com/actions/setup-go#getting-go-version-from-the-gomod-file So for this it's probably `go-version-file: 'go.mod'` ########## main.go: ########## @@ -0,0 +1,45 @@ +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.") +) + +func init() { + // DoFns should be registered with Beam. + beam.RegisterFunction(printAndEmit) +} + +func printAndEmit(element string, emit func(string)) { + fmt.Println(element) + emit(element) +} + +func my_pipeline(scope beam.Scope, input_text string) beam.PCollection { + elements := beam.Create(scope, "Hello", "World!", input_text) + elements = beam.ParDo(scope, printAndEmit, elements) + return elements +} + +func main() { + flag.Parse() + beam.Init() + + ctx := context.Background() + pipeline, scope := beam.NewPipelineWithRoot() + _ = my_pipeline(scope, *input_text) Review Comment: Since we aren't using the return, we can simply avoid mentioning it here. ```suggestion my_pipeline(scope, *input_text) ``` ########## main.go: ########## @@ -14,31 +14,32 @@ 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 init() { + // DoFns should be registered with Beam. + beam.RegisterFunction(printAndEmit) } -func my_pipeline(input_text string) Result { - beam.Init() - - pipeline, scope := beam.NewPipelineWithRoot() +func printAndEmit(element string, emit func(string)) { + fmt.Println(element) Review Comment: import the Beam [log package](https://pkg.go.dev/github.com/apache/beam/sdks/v2/go/pkg/beam/log), and use log.Infoln instead of fmt.Println. This allows the print to be visible from distributed / portable runners. fmt.Println will simply have the output lost to the worker container. ########## main.go: ########## @@ -0,0 +1,45 @@ +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.") +) + +func init() { + // DoFns should be registered with Beam. + beam.RegisterFunction(printAndEmit) +} + +func printAndEmit(element string, emit func(string)) { + fmt.Println(element) + emit(element) +} + +func my_pipeline(scope beam.Scope, input_text string) beam.PCollection { Review Comment: ```suggestion func myPipeline(scope beam.Scope, input_text string) beam.PCollection { ``` Go style has methods/functions use camelcase rather than snake case (snake case is for flags). ########## main.go: ########## @@ -14,31 +14,32 @@ 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 init() { + // DoFns should be registered with Beam. + beam.RegisterFunction(printAndEmit) } -func my_pipeline(input_text string) Result { - beam.Init() - - pipeline, scope := beam.NewPipelineWithRoot() +func printAndEmit(element string, emit func(string)) { + fmt.Println(element) + emit(element) +} +func my_pipeline(scope beam.Scope, input_text string) beam.PCollection { elements := beam.Create(scope, "Hello", "World!", input_text) - elements = beam.ParDo(scope, func(elem string, emit func(string)) { fmt.Println(elem); emit(elem) }, elements) - - return Result{pipeline, scope, elements} + elements = beam.ParDo(scope, printAndEmit, elements) + return elements Review Comment: Style wise, this is a code smell. Simpler to simply return without the variable. ```suggestion return beam.ParDo(scope, printAndEmit, elements) ``` -- 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]
