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

fix linting issues for golangci-lint v2 #187

Merged
merged 3 commits into from
Apr 7, 2025

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 4, 2025

We could suppress this linter, as using w.File.xxx makes it slightly more explicit, but probably fine as well.

Error: atomicwriter/atomicwriter.go:205:11: QF1008: could remove embedded field "File" from selector (staticcheck)
	err := w.File.Sync()
	         ^
Error: mountinfo/mounted_linux_test.go:282:6: QF1001: could apply De Morgan's law (staticcheck)
		if !(tc.isNotExist && errors.Is(err, os.ErrNotExist)) {
		   ^
Error: mountinfo/mounted_linux_test.go:353:8: QF1001: could apply De Morgan's law (staticcheck)
				if !(tc.isNotExist && errors.Is(err, os.ErrNotExist)) {
				   ^
Error: capability/syscall_linux.go:144:2: QF1003: could use tagged switch on data.version (staticcheck)
	if data.version == 1 {
	^
Error: user/user.go:447:16: ST1005: error strings should not be capitalized (staticcheck)
			return nil, fmt.Errorf("Unable to find additional groups %v: %w", additionalGroups, err)
			            ^
Error: user/user.go:471:17: ST1005: error strings should not be capitalized (staticcheck)
				return nil, fmt.Errorf("Unable to find group %s: %w", ag, ErrNoGroupEntries)
				            ^

@thaJeztah thaJeztah changed the title atomicwriter: fix QF1008 (staticcheck) fix linting issues for golangci-lint v2 Apr 4, 2025
@thaJeztah thaJeztah requested a review from kolyshkin April 4, 2025 22:25
err := w.File.Sync()
err := w.Sync()
Copy link
Member Author

Choose a reason for hiding this comment

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

Still a bit on the fence on this one. Yes, it's correct that you can access the embedded field directly, but I think in this case it's better to be explicit. I had a quick peek at the new configuration options, and it looks like we can exclude this "quickfix". From the docs, I think there's various other ones that are disabled by default that we could (should) enable; https://golangci-lint.run/usage/linters/#staticcheck

I'll update the config in the other PR, and drop the commit here.

	Error: mountinfo/mounted_linux_test.go:282:6: QF1001: could apply De Morgan's law (staticcheck)
			if !(tc.isNotExist && errors.Is(err, os.ErrNotExist)) {
			   ^
	Error: mountinfo/mounted_linux_test.go:353:8: QF1001: could apply De Morgan's law (staticcheck)
					if !(tc.isNotExist && errors.Is(err, os.ErrNotExist)) {
					   ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
	Error: capability/syscall_linux.go:144:2: QF1003: could use tagged switch on data.version (staticcheck)
		if data.version == 1 {
		^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
    Error: user/user.go:447:16: ST1005: error strings should not be capitalized (staticcheck)
                return nil, fmt.Errorf("Unable to find additional groups %v: %w", additionalGroups, err)
                            ^
    Error: user/user.go:471:17: ST1005: error strings should not be capitalized (staticcheck)
                    return nil, fmt.Errorf("Unable to find group %s: %w", ag, ErrNoGroupEntries)
                                ^

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the atomicwriter_linting branch from 5e37fa7 to 3c88fb3 Compare April 7, 2025 12:38
@thaJeztah
Copy link
Member Author

Dropped the first commit; I'll bring this one in and rebase the other one.

@thaJeztah thaJeztah merged commit e0b900f into moby:main Apr 7, 2025
20 checks passed
@thaJeztah thaJeztah deleted the atomicwriter_linting branch April 7, 2025 12:42
Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

Believe it or not, but this is almost[1] line-for-line identical my patchset which I prepared last Friday but did not open a PR :)

[1]: except the bit in #188

@thaJeztah
Copy link
Member Author

Ha! Yes, most of the changes seemed reasonable; I was slightly on the fence on the "apply De Morgan's law" changes in 8d553c0, but then again, either before and after would need a short comment (already in place) to outline the logic, so I took that one as "OK, let's make the linter happy".

I didn't like the "use embedded struct directly". At least; not in this specific case (there's situations where it's OK, but this one was not one of them).

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.

3 participants