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.

Sign up for GitHub

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

Jump to bottom

Handle errors discovered by static linter, staticcheck and ineffassign #614

Merged
merged 5 commits into from
Jul 16, 2024
Merged

Handle errors discovered by static linter, staticcheck and ineffassign #614

merged 5 commits into from
Jul 16, 2024

Conversation

LuisPH3
Copy link
Contributor

@LuisPH3 LuisPH3 commented Jul 15, 2024 •
edited
Loading

The goal is still to enable #608

Three kinds:

  • staticcheck detects range checks that are equal to type range. This is superfluous as the compiler already did that
  • deprecated calls, these are bigger than I. Are they really deprecated? The code and the uses seem quite critical to me.

@LuisPH3 LuisPH3 force-pushed the luis/ignored-err-results branch from a4b9b1d to 762ee7a Compare July 15, 2024 09:51
@LuisPH3 LuisPH3 force-pushed the luis/staticcheck-lints branch from 3746be5 to 3d5223a Compare July 15, 2024 09:55
@LuisPH3 LuisPH3 marked this pull request as ready for review July 15, 2024 09:56
facuMH
facuMH previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@facuMH facuMH left a comment

Choose a reason for hiding this comment

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

LGTM

go/ct/gen/interval_solver_test.go Show resolved Hide resolved
simonlechner
simonlechner reviewed Jul 15, 2024
View reviewed changes
Copy link
Contributor

@simonlechner simonlechner left a comment

Choose a reason for hiding this comment

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

Looks good, just one remark.

go/ct/gen/interval_solver_test.go Show resolved Hide resolved
@LuisPH3 LuisPH3 force-pushed the luis/staticcheck-lints branch from 3d5223a to 9185697 Compare July 15, 2024 11:42
@LuisPH3 LuisPH3 requested a review from simonlechner July 15, 2024 11:43
@LuisPH3 LuisPH3 self-assigned this Jul 15, 2024
HerbertJordan
HerbertJordan requested changes Jul 15, 2024
View reviewed changes
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

I do not like those seemingly unmotivated nolint:staticcheck comments. What are the issues there and why can't they be fixed?

go/ct/utils/adapter_test.go Outdated Show resolved Hide resolved
@LuisPH3 LuisPH3 force-pushed the luis/staticcheck-lints branch from 9185697 to 3ed799a Compare July 16, 2024 07:59
@LuisPH3 LuisPH3 requested a review from HerbertJordan July 16, 2024 08:03
Base automatically changed from luis/ignored-err-results to main July 16, 2024 08:53
@LuisPH3 LuisPH3 dismissed facuMH’s stale review July 16, 2024 08:53

The base branch was changed.

LuisPH3 added 4 commits July 16, 2024 11:20
@LuisPH3
Ignore deprecated calls
d92910d 
On a per-call basis, to re-evaluate when introducing new ones.
@LuisPH3
Remove range checks already carried out by the type system
6836dd6
@LuisPH3
Fix double include
191f9b9
@LuisPH3
Migrate golanglint-ci to staticcheck
433f16f 
This linter seems to find more errors and it provides a more expressive ignore comment.
@LuisPH3 LuisPH3 force-pushed the luis/staticcheck-lints branch from 3ed799a to 433f16f Compare July 16, 2024 09:21
@LuisPH3
Copy link
Contributor Author

LuisPH3 commented Jul 16, 2024

I changed the linter from golangci-lint to staticcheck, these are different projects:

  • staticcheck does not report ineffectual assignment, I do prefer the code initializing variables to meaningful values.
  • It has a more expressive ignore comment, fine grain, and with reason comment.

@LuisPH3 LuisPH3 requested a review from facuMH July 16, 2024 12:25
facuMH
facuMH approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@facuMH facuMH left a comment

Choose a reason for hiding this comment

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

descriptive comments detailing the issue and a number of for the issue to fix it are much appreciated !! thanks

HerbertJordan
HerbertJordan approved these changes Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

Thanks, I think this way the purpose of those annotations is way cleaner.

@LuisPH3 LuisPH3 merged commit 700cbfc into main Jul 16, 2024
4 checks passed
@LuisPH3 LuisPH3 deleted the luis/staticcheck-lints branch July 17, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facuMH facuMH facuMH approved these changes

@HerbertJordan HerbertJordan HerbertJordan approved these changes

@simonlechner simonlechner Awaiting requested review from simonlechner

Assignees

@LuisPH3 LuisPH3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LuisPH3 @HerbertJordan @facuMH @simonlechner