Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: clean up some tiny things #1966

Merged
merged 4 commits into from
Nov 26, 2022
Merged

*: clean up some tiny things #1966

merged 4 commits into from
Nov 26, 2022

Conversation

jreut
Copy link
Contributor

@jreut jreut commented Nov 25, 2022

Howdy!

In the course of learning this code, I found a few tiny changes
that I hope you might find useful! I'm happy to split these into
separate pull requests. Or, please feel free to close this if you
don't want drive-by changes like these.

Thank you!


  • internal/cmd: fix an exit code buglet

It seems the previous code intended to interpret the error from
rootCmd.ExecuteContext() as an *exec.ExitError. That's not the way
it worked in practice, though. Instead, the typecheck on
err.(*exec.ExitError) used the error from the call to tracer.Start().
This is due to the scoping rules of if statements.

With this commit, cmd.Do() now attempts to interpret the command
execution error as an *exec.ExitError. It also prints the error
message to stderr, since the caller in ./cmd/sqlc.main doesn't do
that.

See: https://go.dev/ref/spec#Blocks

  • internal/debug: remove Traced global

Previously, some callers of trace.StartRegion() guarded on the
global debug.Traced variable. This guard is unneeded because
runtime/trace.StartRegion() will return a no-op region if tracing
has not been started. That means we can centralize the decision of
whether we want tracing to cmd.Do(). Since we then only have one
reference to debug.Traced, we can just delete it, since it seems
it was a shorthand for (debug.Debug.Trace != "").

See: https://cs.opensource.google/go/go/+/refs/tags/go1.19.3:src/runtime/trace/annotation.go;l=153-155

  • internal/codegen: don't copy protobuf locks

This patch silences the output of "go vet ./...", which used to say this:

# github.com/kyleconroy/sqlc/internal/codegen/json
internal/codegen/json/gen.go:24:12: return copies lock value: github.com/kyleconroy/sqlc/internal/plugin.JSONCode contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
internal/codegen/json/gen.go:26:11: return copies lock value: github.com/kyleconroy/sqlc/internal/plugin.JSONCode contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
internal/codegen/json/gen.go:30:10: return copies lock value: github.com/kyleconroy/sqlc/internal/plugin.JSONCode contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
# github.com/kyleconroy/sqlc/internal/codegen/golang
internal/codegen/golang/imports.go:68:9: range var strct copies lock: github.com/kyleconroy/sqlc/internal/codegen/golang.Struct contains github.com/kyleconroy/sqlc/internal/plugin.Identifier contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
internal/codegen/golang/result.go:94:30: call of append copies lock value: github.com/kyleconroy/sqlc/internal/codegen/golang.Struct contains github.com/kyleconroy/sqlc/internal/plugin.Identifier contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
internal/codegen/golang/result.go:208:11: range var s copies lock: github.com/kyleconroy/sqlc/internal/codegen/golang.Struct contains github.com/kyleconroy/sqlc/internal/plugin.Identifier contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex

Also, add go vet to CI.

  • internal/cmd: use *cobra.Command.RunE everywhere

Errors are handled by the root command's execution, so we don't
need to os.Exit() in any child commands.

It seems the previous code intended to interpret the error from
rootCmd.ExecuteContext() as an *exec.ExitError. That's not the way
it worked in practice, though. Instead, the typecheck on
err.(*exec.ExitError) used the error from the call to tracer.Start().
This is due to the scoping rules of if statements.

With this commit, cmd.Do() now attempts to interpret the command
execution error as an *exec.ExitError. It also prints the error
message to stderr, since the caller in ./cmd/sqlc.main doesn't do
that.

See: https://go.dev/ref/spec#Blocks
Previously, some callers of trace.StartRegion() guarded on the
global debug.Traced variable. This guard is unneeded because
runtime/trace.StartRegion() will return a no-op region if tracing
has not been started. That means we can centralize the decision of
whether we want tracing to cmd.Do(). Since we then only have one
reference to debug.Traced, we can just delete it, since it seems
it was a shorthand for (debug.Debug.Trace != "").

See: https://cs.opensource.google/go/go/+/refs/tags/go1.19.3:src/runtime/trace/annotation.go;l=153-155
This patch silences the output of "go vet ./...", which used to say this:

	# github.com/kyleconroy/sqlc/internal/codegen/json
	internal/codegen/json/gen.go:24:12: return copies lock value: github.com/kyleconroy/sqlc/internal/plugin.JSONCode contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
	internal/codegen/json/gen.go:26:11: return copies lock value: github.com/kyleconroy/sqlc/internal/plugin.JSONCode contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
	internal/codegen/json/gen.go:30:10: return copies lock value: github.com/kyleconroy/sqlc/internal/plugin.JSONCode contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
	# github.com/kyleconroy/sqlc/internal/codegen/golang
	internal/codegen/golang/imports.go:68:9: range var strct copies lock: github.com/kyleconroy/sqlc/internal/codegen/golang.Struct contains github.com/kyleconroy/sqlc/internal/plugin.Identifier contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
	internal/codegen/golang/result.go:94:30: call of append copies lock value: github.com/kyleconroy/sqlc/internal/codegen/golang.Struct contains github.com/kyleconroy/sqlc/internal/plugin.Identifier contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
	internal/codegen/golang/result.go:208:11: range var s copies lock: github.com/kyleconroy/sqlc/internal/codegen/golang.Struct contains github.com/kyleconroy/sqlc/internal/plugin.Identifier contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex

Also, add go vet to CI.
Errors are handled by the root command's execution, so we don't
need to os.Exit() in any child commands.
@jreut jreut changed the title misc *: clean up some tiny things Nov 25, 2022
@kyleconroy kyleconroy merged commit c8c0233 into sqlc-dev:main Nov 26, 2022
@jreut jreut deleted the misc branch November 26, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants