lostluck commented on a change in pull request #13484:
URL: https://github.com/apache/beam/pull/13484#discussion_r536493356
##########
File path: sdks/go/pkg/beam/runners/universal/runnerlib/execute.go
##########
@@ -94,11 +94,16 @@ func Execute(ctx context.Context, p *pipepb.Pipeline,
endpoint string, opt *JobO
}
err = WaitForCompletion(ctx, client, jobID)
- res, err := newUniversalPipelineResult(ctx, jobID, client)
- if err != nil {
- return presult, err
+ res, presultErr := newUniversalPipelineResult(ctx, jobID, client)
+ if presultErr != nil {
+ if err != nil {
+ return presult, errors.Wrap(err, presultErr.Error())
+ } else {
+ return presult, presultErr
+ }
+ } else {
+ presult = res
}
Review comment:
In Go, there's no reason to try to have a single return, or maintain an
else.
In Go, prefer having the happiest path have the least indentation, and
strive to return to that lowest level. Simply return, and avoid the extra
indentation of an else.
An if else block implies that either clause can be executed and those
possibilities need to be kept in mind when reading later code. If the if
returns (or continues, or breaks, as appropriate), there's no need for an else,
as the code in the If never interacts with anything after it.
```suggestion
res, presultErr := newUniversalPipelineResult(ctx, jobID, client)
if presultErr != nil {
if err != nil {
return presult, errors.Wrap(err, presultErr.Error())
}
return presult, presultErr
}
return res, err
```
This in combination with my other suggestion below to remove the other line.
##########
File path: sdks/go/pkg/beam/runners/universal/runnerlib/execute.go
##########
@@ -94,11 +94,16 @@ func Execute(ctx context.Context, p *pipepb.Pipeline,
endpoint string, opt *JobO
}
err = WaitForCompletion(ctx, client, jobID)
- res, err := newUniversalPipelineResult(ctx, jobID, client)
- if err != nil {
- return presult, err
+ res, presultErr := newUniversalPipelineResult(ctx, jobID, client)
+ if presultErr != nil {
+ if err != nil {
+ return presult, errors.Wrap(err, presultErr.Error())
+ } else {
+ return presult, presultErr
+ }
+ } else {
+ presult = res
}
- presult = res
return presult, err
}
Review comment:
```suggestion
}
```
cleanup of the old return that doesn't need to be preserved.
----------------------------------------------------------------
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:
[email protected]
