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

GO code staticcheck cleanup #1038

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GO code staticcheck cleanup #1038

wants to merge 1 commit into from

Conversation

nanjj
Copy link
Contributor

@nanjj nanjj commented Jul 23, 2024

Ignored code generated files and fixed the left checks

@nanjj nanjj requested a review from stgraber as a code owner July 23, 2024 04:13
Ignored code generated files and fixed the left checks

Signed-off-by: JUN JIE NAN <[email protected]>
"github.com/lxc/incus/v6/client"
incus "github.com/lxc/incus/v6/client"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you ran goimports or something like that which has a tendency to go mess with our import names, can you remove all of those from your commit?

Comment on lines -430 to +432
} else { //nolint:staticcheck // (keep the empty branch for the comment)
//nolint:all
//lint:ignore SA9003 keep the empty branch for the comment
} else { //nolint:all
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what's going on here, that change looks off.

@@ -28,6 +28,7 @@ func Reset(path string, imports []string, buildComment string, iface bool) error
content := fmt.Sprintf(`%spackage %s

// The code below was generated by %s - DO NOT EDIT!
//lint:file-ignore U1000,SA4006 Ignore staticcheck
Copy link
Member

Choose a reason for hiding this comment

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

So just because the files are auto-generated doesn't mean that issues/lints aren't worth fixing.

It just means that rather than fixing the issues directly in the .mapper.go files, those issues need to instead be fixed in the generator code.

@@ -1,6 +1,8 @@
// Package response contains helpers for rendering HTTP responses.
//
//nolint:deadcode,unused
//nolint:all
Copy link
Member

Choose a reason for hiding this comment

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

Why ignoring all lints?

@@ -332,6 +332,8 @@ func (c *Config) getConnectionArgs(name string) (*incus.ConnectionArgs, error) {
// Golang has deprecated all methods relating to PEM encryption due to a vulnerability.
// However, the weakness does not make PEM unsafe for our purposes as it pertains to password protection on the
// key file (client.key is only readable to the user in any case), so we'll ignore deprecation.
//nolint:all
Copy link
Member

Choose a reason for hiding this comment

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

Why turn off all linting?

@@ -366,6 +368,8 @@ func (c *Config) getConnectionArgs(name string) (*incus.ConnectionArgs, error) {
return nil, fmt.Errorf("Unsupported key type: %T", sshKey)
}
} else {
//nolint:all
Copy link
Member

Choose a reason for hiding this comment

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

Why turn off all linting?

@@ -241,7 +241,8 @@ func (c *cmdAdminRecover) Run(cmd *cobra.Command, args []string) error {
// Send /internal/recover/import request to the daemon.
// Don't lint next line with gosimple. It says we should convert reqValidate directly to an RecoverImportPost
// because their types are identical. This is less clear and will not work if either type changes in the future.
reqImport := recover.ImportPost{ //nolint:gosimple
//nolint:all
Copy link
Member

Choose a reason for hiding this comment

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

Why turn off all linting?

@@ -593,6 +593,8 @@ func NotifyHeartbeat(state *state.State, gateway *Gateway) {
// Wait for heartbeat to finish and then release.
// Ignore staticcheck "SA2001: empty critical section" because we want to wait for the lock.
gateway.HeartbeatLock.Lock()
//nolint:all
Copy link
Member

Choose a reason for hiding this comment

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

Why turn off all linting?

@@ -593,6 +593,8 @@ func NotifyHeartbeat(state *state.State, gateway *Gateway) {
// Wait for heartbeat to finish and then release.
// Ignore staticcheck "SA2001: empty critical section" because we want to wait for the lock.
gateway.HeartbeatLock.Lock()
//nolint:all
//lint:ignore SA2001 we want to wait for the lock
Copy link
Member

Choose a reason for hiding this comment

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

The comment above (// Ignore staticcheck ...) can be removed given this one just repeats it

@stgraber stgraber added the Incomplete Waiting on more information from reporter label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Incomplete Waiting on more information from reporter
Development

Successfully merging this pull request may close these issues.

2 participants