Fix linting errors #3189

MDrakos · 2024-03-21T17:03:31Z

DX-2358 Fix the hundreds of linter warnings/errors in the repo.

mitchell-as

This is some great work! Don't be discouraged with the number of change requests because there are bound to be some in 266 changed files :)

There are a handful of minor things to address, but I think the main thing I'd like to bring up is that I would personally prefer avoiding //nolint: at all costs. Writing code to pander to a linter is not ideal unless we absolutely cannot avoid it. For example instead of tagging a function as //nolint:unused because it's only used on Linux, create a _linux.go file and move that tagged function into it (and then untag it).

cmd/state-svc/autostart/autostart.go

mitchell-as · 2024-03-26T18:32:32Z

cmd/state-svc/internal/messages/messages.go

@@ -76,7 +76,8 @@ func (m *Messages) Check(command string, flags []string) ([]*graph.MessageInfo,
 	}
 	allMessages := cacheValue.([]*graph.MessageInfo)

-	conditionParams := &(*m.baseParams) // copy
+	// copy the base params
+	conditionParams := &(*m.baseParams) //nolint:staticcheck


I think a better solution would be to use conditionParams := m.baseParams // copy and then change *conditionParams to conditionParams on line 89, and change conditionParams to &conditionParams on line 92.

m.baseparams is a pointer to a struct, so conditionParams := m.basParams will just be a copy of the pointer. I believe dereferencing here is what we want so I've updated it to conditionParams := *m.baseParams. Either way, the current code on master is not creating a copy of the struct.

Yes, you are correct. I made a typo; I missed the '*' dereference. You did what I had in mind.

cmd/state/autoupdate.go

internal/analytics/client/sync/client.go

test/integration/checkout_int_test.go

test/integration/performance_expansion_int_test.go

test/integration/run_int_test.go

excludes.txt

internal/subshell/subshell_test.go

MDrakos · 2024-03-27T20:26:21Z

I've cross-referenced the integration test failures here with our latest nightly failures and it seem to line up, aside from timeouts on Windows.

Thanks for the great review. When dealing with all of these linting errors I started applying quick/lazy fixes as there were just so many of them. A second set of eyes really helped. Hopefully we can be more diligent and stay on top of these in the future.

mitchell-as

Almost there! Just a few more things to address.

internal/logging/rotate.go

pkg/platform/model/vcs.go

internal/fileutils/fileutils_win.go

internal/keypairs/local_load_test.go

mitchell-as · 2024-03-27T21:00:23Z

internal/captain/command.go

@@ -230,7 +227,9 @@ func NewHiddenShimCommand(name string, prime primer, flags []*Flag, args []*Argu
 	}

 	cmd.cobra.SetHelpFunc(func(_ *cobra.Command, args []string) {
-		cmd.execute(cmd, args)
+		if err := cmd.execute(cmd, args); err != nil {
+			panic(err)


I see. GitHub didn't give me that context. Since this is an established pattern, we can keep it. I consider this double-checked!

internal/svcctl/svcctl.go

mitchell-as · 2024-03-27T21:25:04Z

internal/uniqid/uniqid.go

@@ -190,7 +189,9 @@ func writeFile(filePath string, data []byte) error {
 		if err := os.Chmod(filePath, 0644); err != nil {
 			return fmt.Errorf("os.Chmod %s failed: %w", filePath, err)
 		}
-		defer os.Chmod(filePath, stat.Mode().Perm())
+		defer func() {
+			rerr = os.Chmod(filePath, stat.Mode().Perm())


Again, this may end up overwriting an existing error, so perhaps use

if rerr != nil { rerr = os.Chmod(...) }

mitchell-as · 2024-03-27T21:40:17Z

cmd/state-svc/internal/messages/messages.go

@@ -76,7 +76,8 @@ func (m *Messages) Check(command string, flags []string) ([]*graph.MessageInfo,
 	}
 	allMessages := cacheValue.([]*graph.MessageInfo)

-	conditionParams := &(*m.baseParams) // copy
+	// copy the base params
+	conditionParams := &(*m.baseParams) //nolint:staticcheck


Yes, you are correct. I made a typo; I missed the '*' dereference. You did what I had in mind.

mitchell-as · 2024-03-27T21:41:44Z

internal/osutils/termsize/termsize.go

 		uintptr(0),
 		uintptr(syscall.TIOCGWINSZ),
 		uintptr(unsafe.Pointer(&ws)))
+	if err != 0 {


That works. Our integration tests should probably catch it since they fail if there are any errors in the logs.

mitchell-as · 2024-03-27T21:44:53Z

pkg/platform/runtime/buildexpression/merge/merge_test.go

@@ -108,6 +107,7 @@ runtime = solve(
 	],
 	requirements = [
 		Req(name = "language/perl"),
+		Req(name = "language/perl/JSON"),


Thanks for double-checking. You can leave it as-is.

mitchell-as · 2024-03-28T17:49:21Z

We overlooked two lint errors:

https://github.com/ActiveState/cli/blob/DX-2358/internal/subshell/subshell_win_test.go#L59

Error return value of pjfile.Persist is not checked (errcheck)

https://github.com/ActiveState/cli/blob/DX-2358/internal/subshell/subshell_win_test.go#L89

Error return value of pjfile.Persist is not checked (errcheck)

MDrakos · 2024-03-28T18:23:15Z

We overlooked two lint errors:

https://github.com/ActiveState/cli/blob/DX-2358/internal/subshell/subshell_win_test.go#L59

Error return value of pjfile.Persist is not checked (errcheck)

https://github.com/ActiveState/cli/blob/DX-2358/internal/subshell/subshell_win_test.go#L89

Error return value of pjfile.Persist is not checked (errcheck)

Thank you for catching this. Meant to run the linter again on Windows myself but I'm glad you did.

mitchell-as

Yay!

MDrakos added 4 commits March 19, 2024 09:37

Initial batch of changes

9286e50

Another round of changes

9f6eb7e

Fix more linting warnings and errors

d97380b

Merge branch 'version/0-44-0-RC1' into DX-2358

8c13f2c

MDrakos changed the base branch from master to version/0-44-0-RC1 March 21, 2024 17:18

MDrakos added 9 commits March 21, 2024 10:25

Add back apy_installer.go

c1d6e37

Add back apy_installer.go

07630d4

Add back missing function

ec2da7b

Add nolint comment

ee86425

Fix unit tests

675c87d

Fix unit test on Windows

f200a75

Fix Windows specific errors

76b8437

Fix linux specific issues

80f288b

Handle errors in defer

910340c

MDrakos added the Test: all label Mar 21, 2024

MDrakos added 2 commits March 21, 2024 16:08

Remove legacy config handling

db84c83

Attempt to fix linux integration tests

80ed9ee

MDrakos added Test: run Test: uninstall and removed Test: all labels Mar 22, 2024

MDrakos added 2 commits March 22, 2024 10:25

Try increasing timeout

6bdd42f

Remove language from script

e7ed82e

MDrakos added Test: all and removed Test: run Test: uninstall labels Mar 22, 2024

Merge branch 'version/0-44-0-RC1' into DX-2358

9404e2e

MDrakos added Test: activate and removed Test: all labels Mar 22, 2024

MDrakos added 2 commits March 22, 2024 13:27

Revert syscall change

a7b5d07

Don't overwrite error on Windows

aadecbe

Fix checkout integration test

bcd862a

MDrakos added Test: all and removed Test: prepare Test: install-scripts Test: installer Test: checkout Test: remote-installer Test: interrupt labels Mar 22, 2024

MDrakos added 3 commits March 22, 2024 17:20

Debug OLE error

3deb882

Ignore log errors on Windows

12c2272

Expect log error

e31feed

MDrakos marked this pull request as ready for review March 25, 2024 23:04

MDrakos added 2 commits March 26, 2024 09:57

Tag log ignores for follow up

912f832

Merge branch 'version/0-44-0-RC1' into DX-2358

9c0a388

MDrakos requested a review from mitchell-as March 26, 2024 18:12

mitchell-as requested changes Mar 26, 2024

View reviewed changes

MDrakos added 2 commits March 27, 2024 10:00

Remove helper files

388b5d0

Address PR review comments

f86370f

MDrakos requested a review from mitchell-as March 27, 2024 20:26

mitchell-as requested changes Mar 27, 2024

View reviewed changes

MDrakos added 2 commits March 28, 2024 10:14

Address additional PR review comments

1817246

Update another defer

b70afd2

Catch test errors

fb3e40c

Use errs.Pack

8afe1b6

mitchell-as approved these changes Mar 28, 2024

View reviewed changes

MDrakos merged commit af8a8b9 into version/0-44-0-RC1 Mar 28, 2024
4 of 7 checks passed

MDrakos deleted the DX-2358 branch March 28, 2024 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix linting errors #3189

Fix linting errors #3189

MDrakos commented Mar 21, 2024 •

edited by github-actions bot

Loading

mitchell-as left a comment

mitchell-as Mar 26, 2024

MDrakos Mar 27, 2024 •

edited

Loading

mitchell-as Mar 27, 2024

MDrakos commented Mar 27, 2024

mitchell-as left a comment

mitchell-as Mar 27, 2024

mitchell-as Mar 27, 2024

mitchell-as Mar 27, 2024

mitchell-as Mar 27, 2024

mitchell-as Mar 27, 2024

mitchell-as commented Mar 28, 2024

MDrakos commented Mar 28, 2024

mitchell-as left a comment

Fix linting errors #3189

Fix linting errors #3189

Conversation

MDrakos commented Mar 21, 2024 • edited by github-actions bot Loading

mitchell-as left a comment

Choose a reason for hiding this comment

mitchell-as Mar 26, 2024

Choose a reason for hiding this comment

MDrakos Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

mitchell-as Mar 27, 2024

Choose a reason for hiding this comment

MDrakos commented Mar 27, 2024

mitchell-as left a comment

Choose a reason for hiding this comment

mitchell-as Mar 27, 2024

Choose a reason for hiding this comment

mitchell-as Mar 27, 2024

Choose a reason for hiding this comment

mitchell-as Mar 27, 2024

Choose a reason for hiding this comment

mitchell-as Mar 27, 2024

Choose a reason for hiding this comment

mitchell-as Mar 27, 2024

Choose a reason for hiding this comment

mitchell-as commented Mar 28, 2024

MDrakos commented Mar 28, 2024

mitchell-as left a comment

Choose a reason for hiding this comment

MDrakos commented Mar 21, 2024 •

edited by github-actions bot

Loading

MDrakos Mar 27, 2024 •

edited

Loading