Skip to content

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

Merged
merged 1 commit into from
Apr 14, 2025
Merged

tests: extend linting #54

merged 1 commit into from
Apr 14, 2025

Conversation

murfffi
Copy link
Member

@murfffi murfffi commented Apr 14, 2025

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

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
Copy link

@gemini-code-assist gemini-code-assist bot left a 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 integrated staticcheck 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 and go.sum to reflect new dependencies and Go version.
  • Code Cleanup: Replaced explicit Close() calls with closeQuietly() to handle potential errors during resource closing in dbmgr.go.

Changelog

Click here to see the changelog
  • .golangci.yml
    • Upgraded the configuration file to version 2 for golangci-lint.
  • README.md
    • Removed the specific Go version from the Docker run command, using golang instead of golang:1.23.
  • 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, and golang.org/x/tools as indirect dependencies.
    • Added honnef.co/go/tools as an indirect dependency and a tool dependency for staticcheck.
  • go.sum
    • Added checksums for new and updated dependencies, including github.com/BurntSushi/toml and honnef.co/go/tools.
  • internal/gen/dbmgr.go
    • Added io import.
    • Replaced explicit colRows.Close() with closeQuietly(colRows) at line 158.
    • Replaced explicit stmt.Close() with closeQuietly(stmt) at line 179.
    • Replaced explicit finStmt.Close() with closeQuietly(finStmt) at line 223.
    • Added closeQuietly function to handle closing resources without error checking.
  • internal/gen/gen.go
    • Removed go:embed directive and variable declaration for main.go.tpl at lines 22-23.
  • internal/gen/main.go.tpl.go
    • Moved the content of main.go.tpl to a constant string mainTpl within a .go file at line 6.
    • Added a comment explaining the reason for moving the template to a constant.
  • internal/shell/cmd_build.go
    • Replaced c.CompileCommand.compile with c.compile at line 60.
  • internal/shell/cmd_build_test.go
    • Replaced cmd.CompileCommand.generator with cmd.generator at line 65.
  • internal/shell/commands.go
    • Replaced c.CompileCommand.compile with c.compile at line 17.
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

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

Copy link

@gemini-code-assist gemini-code-assist bot left a 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: The closeQuietly function ignores the error returned by c.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 named closeQuietly, logging the error might be counterintuitive, but consider the trade-offs.
  • Redundant os.RemoveAll call: The os.RemoveAll function is called in a defer statement within the compile 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 explicit os.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 by db.QueryContext and wrt.PrepareContext are wrapped using fmt.Errorf. However, the error returned by rows.Scan is not wrapped. Consider wrapping the error returned by rows.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.

Comment on lines +241 to +243
func closeQuietly(c io.Closer) {
// Can't use sclerr since dbmgr is standalone.
_ = c.Close()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)
	}
}

@murfffi murfffi changed the title tests: extend linting 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 tests: extend linting 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 Apr 14, 2025
@murfffi murfffi changed the title tests: extend linting 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 tests: extend linting Apr 14, 2025
@murfffi murfffi merged commit 6dddd45 into main Apr 14, 2025
4 checks passed
@murfffi murfffi deleted the tests/template_lint branch April 14, 2025 18:56
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.

1 participant