-
Notifications
You must be signed in to change notification settings - Fork 0
tests: extend linting #54
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
Conversation
Extends lint coverage by: - upgrading to golangci-lint v2 - adding staticcheck as an optional tool with go tool - move main.go.tpl from a file included with go:embed to a constant string so that golangci-lint can find some issue via staticcheck SA1001 - resolve issues found by the update lint stack - move to 1.24 for go tool support Testing Done: added an error to template and confirmed that it was detected; and couldn't be detected before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @murfffi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on extending the linting coverage for the project. I've upgraded golangci-lint
to v2, added staticcheck
as an optional tool, and resolved issues found by the updated linting stack. Additionally, I've moved to Go 1.24 for tool support and addressed a staticcheck SA1001 issue by moving main.go.tpl
content to a constant string. The changes also include updates to go.mod
and go.sum
to reflect the new dependencies and Go version.
Highlights
- Linting Upgrade: Upgraded to
golangci-lint
v2 and integratedstaticcheck
for enhanced static analysis. - Code Modernization: Moved
main.go.tpl
content to a constant string to enable static analysis and resolved related linting issues. - Go Version Update: Updated the Go version to 1.24 for improved tool support.
- Dependency Updates: Updated
go.mod
andgo.sum
to reflect new dependencies and Go version. - Code Cleanup: Replaced explicit
Close()
calls withcloseQuietly()
to handle potential errors during resource closing indbmgr.go
.
Changelog
Click here to see the changelog
- .golangci.yml
- Upgraded the configuration file to version 2 for
golangci-lint
.
- Upgraded the configuration file to version 2 for
- README.md
- Removed the specific Go version from the Docker run command, using
golang
instead ofgolang:1.23
.
- Removed the specific Go version from the Docker run command, using
- go.mod
- Upgraded the Go version to 1.24.1.
- Added
github.com/BurntSushi/toml
as an indirect dependency. - Added
golang.org/x/exp/typeparams
,golang.org/x/mod
,golang.org/x/sync
, andgolang.org/x/tools
as indirect dependencies. - Added
honnef.co/go/tools
as an indirect dependency and a tool dependency forstaticcheck
.
- go.sum
- Added checksums for new and updated dependencies, including
github.com/BurntSushi/toml
andhonnef.co/go/tools
.
- Added checksums for new and updated dependencies, including
- internal/gen/dbmgr.go
- Added
io
import. - Replaced explicit
colRows.Close()
withcloseQuietly(colRows)
at line 158. - Replaced explicit
stmt.Close()
withcloseQuietly(stmt)
at line 179. - Replaced explicit
finStmt.Close()
withcloseQuietly(finStmt)
at line 223. - Added
closeQuietly
function to handle closing resources without error checking.
- Added
- internal/gen/gen.go
- Removed
go:embed
directive and variable declaration formain.go.tpl
at lines 22-23.
- Removed
- internal/gen/main.go.tpl.go
- Moved the content of
main.go.tpl
to a constant stringmainTpl
within a.go
file at line 6. - Added a comment explaining the reason for moving the template to a constant.
- Moved the content of
- internal/shell/cmd_build.go
- Replaced
c.CompileCommand.compile
withc.compile
at line 60.
- Replaced
- internal/shell/cmd_build_test.go
- Replaced
cmd.CompileCommand.generator
withcmd.generator
at line 65.
- Replaced
- internal/shell/commands.go
- Replaced
c.CompileCommand.compile
withc.compile
at line 17.
- Replaced
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A linter's keen eye,
Finds bugs that slip on by,
Code's cleaner and bright,
With staticcheck's light,
A better program, oh my!
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request enhances the linting process by upgrading golangci-lint
, adding staticcheck
, and resolving issues found by the updated lint stack. The changes also include moving main.go.tpl
to a constant string for better linting and updating the Go tool support to 1.24. Overall, the changes improve code quality and maintainability.
Summary of Findings
- Error handling in
closeQuietly
: ThecloseQuietly
function ignores the error returned byc.Close()
. While the intention is to handle the error silently, it might be beneficial to log the error for debugging purposes, especially in production environments. Since the function is namedcloseQuietly
, logging the error might be counterintuitive, but consider the trade-offs. - Redundant
os.RemoveAll
call: Theos.RemoveAll
function is called in a defer statement within thecompile
function. This ensures that the temporary directory is cleaned up when the function exits. However, if the compilation process fails before the defer statement is executed, the temporary directory might not be cleaned up. Consider adding an explicitos.RemoveAll
call before returning an error to ensure that the temporary directory is always cleaned up. - Inconsistent error handling: In the
SimpleCopyWithInsert
function, errors returned bydb.QueryContext
andwrt.PrepareContext
are wrapped usingfmt.Errorf
. However, the error returned byrows.Scan
is not wrapped. Consider wrapping the error returned byrows.Scan
to provide more context.
Merge Readiness
The pull request introduces significant improvements to the linting process and resolves several issues. While the changes are generally well-implemented, there are a few areas that could be improved, such as error handling in closeQuietly
and inconsistent error wrapping in SimpleCopyWithInsert
. Addressing these issues would further enhance the code's robustness and maintainability. I am unable to approve this pull request, and recommend that others review and approve this code before merging. Given the medium
severity comments, I recommend that the pull request not be merged until those are addressed.
func closeQuietly(c io.Closer) { | ||
// Can't use sclerr since dbmgr is standalone. | ||
_ = c.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error returned by c.Close()
is ignored here. While the intention is to handle the error silently, it might be beneficial to log the error for debugging purposes, especially in production environments. Since the function is named closeQuietly
, logging the error might be counterintuitive, but consider the trade-offs.
func closeQuietly(c io.Closer) {
// Can't use sclerr since dbmgr is standalone.
err := c.Close()
if err != nil {
log.Printf("Error closing: %v", err)
}
}
Extends lint coverage by:
upgrading to golangci-lint v2
adding staticcheck as an optional tool with go tool
moving main.go.tpl from a file included with go:embed to a constant string so that golangci-lint can find some issue via staticcheck SA1001
resolving issues found by the update lint stack
moving to 1.24 for go tool support
Testing Done: added an error to template and confirmed that it was detected; and couldn't be detected before