Switch from golint to staticcheck #457

carleeto · 2022-02-10T19:31:00Z

Description

This PR switches static checking from the deprecated tool golint to staticcheck

Motivation & context

It switches static linting from a deprecated tool to a recommended one.

Fixes #448

Type of change

Refactoring/debt (improvement to code design or tooling without changing behaviour)

Checklist:

I have read the CONTRIBUTING document.
My change needed additional tests
- I have added tests to cover my changes.
My change requires a change to the documentation. (doesn't require a change, but I have changed CONTRIBUTING.md)
- I have updated the documentation accordingly. (to make it easier for future authors who update dependencies)
I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

run_test.go:618:6: func passingStepDefWithoutReturn is unused (U1000)

suite.go:421:6: func isEmptyFeature is unused (U1000)

test_context_test.go:45:66: unnecessary use of fmt.Sprintf (S1039) test_context_test.go:46:61: unnecessary use of fmt.Sprintf (S1039)

https://github.com/cucumber/godog/runs/5146601108?check_suite_focus=true#step:7:28

carleeto · 2022-02-10T20:15:58Z

I've enabled staticcheck in CI. There are a few errors.
Before doing this, go vet was failing on go1.16.

Plan:
First, get staticcheck back to green by disabling failing checks.
Next, understand why go vet failed on go 1.16
Finally, ensure CI is green and reopen the PR

mattwynne · 2022-02-10T20:16:47Z

I'm currently pairing with @carleeto on this. It's tricky for him to get feedback as I have to press the "Approve" button each time he pushes! I'm confident he's going to be a useful member of the team so I've pre-empted the commit-bit bot and added him to the cuucmber/committers team manually.

https://github.com/cucumber/godog/runs/5147133955?check_suite_focus=true#step:6:17

vearutop · 2022-02-10T20:20:10Z

Maybe let's try downloading staticcheck from https://github.com/dominikh/go-tools/releases/download/2021.1.2/staticcheck_linux_amd64.tar.gz instead of building it, it might be faster and less invasive.

mattwynne · 2022-02-10T20:23:13Z

We seem to have a weird issue where the go vet command is failing in CI complaining about a missing go.sum entry which we don't think is missing - any idea why that would be happening @vearutop?

We've tried reproducing locally on a v1.16 go and it seems to work fine.

.github/workflows/test.yml

Makefile

.github/workflows/test.yml

Co-authored-by: Viacheslav Poturaev <nanopeni@gmail.com>

carleeto · 2022-02-11T21:44:39Z

@vearutop, @mattwynne: go vet checks in the 1.16 build are failing because the _examples folder is its own Go module (the folder has its own go.mod and go sum files - https://github.com/cucumber/godog/blob/main/_examples/go.mod). I'd like to understand the reasoning behind this.

codecov · 2022-02-11T21:54:01Z

Codecov Report

Merging #457 (8594a6c) into main (1c91e6f) will increase coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
+ Coverage   81.47%   81.58%   +0.11%     
==========================================
  Files          27       27              
  Lines        2186     2183       -3     
==========================================
  Hits         1781     1781              
+ Misses        312      309       -3     
  Partials       93       93

Impacted Files	Coverage Δ
suite.go	`87.55% <ø> (+1.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c91e6f...8594a6c. Read the comment docs.

carleeto · 2022-02-11T22:05:05Z

@vearutop, @mattwynne: go vet checks in the 1.16 build are failing because the _examples folder is its own Go module (the folder has its own go.mod and go sum files - https://github.com/cucumber/godog/blob/main/_examples/go.mod). I'd like to understand the reasoning behind this.

I think I understand why - its an example of how a user would use godog, so it needed to be its own sub-module. Am I right?

Also add a note to CONTRIBUTING.md about the _examples module

Co-authored-by: Viacheslav Poturaev <nanopeni@gmail.com>

mattwynne · 2022-02-12T01:16:56Z

@vearutop, @mattwynne: go vet checks in the 1.16 build are failing because the _examples folder is its own Go module (the folder has its own go.mod and go sum files - https://github.com/cucumber/godog/blob/main/_examples/go.mod). I'd like to understand the reasoning behind this.

I think I understand why - its an example of how a user would use godog, so it needed to be its own sub-module. Am I right?

I would have expected that each folder within the examples directory would have it's own go.mod, since I think each example is supposed to be like a separate project.

So, yeah, I don't really understand the way it is right now.

carleeto · 2022-02-12T02:50:01Z

I have stored the linux binary for staticcheck in a bin folder at the repo root. Happy to move it somewhere else if necessary.

mdelapenya · 2022-02-12T07:33:59Z

Have you thought using https://pre-commit.com and handle the static checks there? Then the same checks would be done at CI level than at local one

On the other hand, storing the binary in the project seems not a common pattern. Go has the tools.go approach, as described here: https://play-with-go.dev/tools-as-dependencies_go115_en/

carleeto · 2022-02-12T23:55:33Z

I started with tools.go and switched to storing a binary based on a suggestion by @vearutop.

Personally, I prefer the tools.go approach, because it guarantees that CI and developers' machines run the exact same checks on all supported platforms.

That said, if I could get a decision on the approach to be used, I'll go with it.

carleeto · 2022-02-13T00:03:18Z

Have you thought using https://pre-commit.com and handle the static checks there? Then the same checks would be done at CI level than at local one

If I understand correctly, the intent is to run a pre-commit check so that code that is committed is well formed.

If that's the case, then the same can be accomplished using vanilla .githooks and Go.

From what I understand, pre-commit.com would add a python dependency, which doesn't really seem to add value.

mdelapenya · 2022-02-13T08:19:53Z

If I understand correctly, the intent is to run a pre-commit check so that code that is committed is well formed.

The intend would be running the very same "hooks" across any member of the team including contributors. And a hook could be running whatever thing: we use it to check your shell scripts format, yml indent... Or even run the unit test if needed (depending of course the amount of time they take).

If that's the case, then the same can be accomplished using vanilla .githooks and Go.

From what I understand, pre-commit.com would add a python dependency, which doesn't really seem to add value.

mattwynne · 2022-02-14T17:38:49Z

Thanks for the suggestion @mdelapenya!

I suggest that, to keep things moving, we keep the scope of this PR to moving from gotlint to staticcheck and we deal with improving how and when we run the linter in a separate PR.

mattwynne · 2022-02-14T17:39:32Z

@vearutop do you have any comments on the way Carl's include the binary for staticcheck? Was that what you meant, or did you mean to script a download of the binary during the build process?

vearutop

Looks good to me 👍

vearutop · 2022-02-14T22:45:55Z

go vet checks in the 1.16 build are failing because the _examples folder is its own Go module (the folder has its own go.mod and go sum files - https://github.com/cucumber/godog/blob/main/_examples/go.mod). I'd like to understand the reasoning behind this.

Examples try to represent realistic cases and for that they have extra dependencies (for example github.com/go-sql-driver/mysql), these dependencies are redundant for godog itself, that's the reason why examples have their own module.

In the past each example had its own module and a dependency on already released version of godog, but that was not very convenient because examples had to be updated separately from godog.

mattwynne · 2022-02-15T21:37:57Z

Thanks @carleeto and welcome to the Godog contributors team! 🎉

carleeto · 2022-02-16T04:21:40Z

Cheers @mattwynne. Thanks @vearutop and @mdelapenya for the review too.

carleeto added 6 commits February 11, 2022 09:03

Switch from golint to staticcheck

0bed6e1

Remove unused function

90856a8

run_test.go:618:6: func passingStepDefWithoutReturn is unused (U1000)

Remove unused function

51b43f4

suite.go:421:6: func isEmptyFeature is unused (U1000)

Fix unnecessary use of fmt.Sprintf

8e1c1a3

test_context_test.go:45:66: unnecessary use of fmt.Sprintf (S1039) test_context_test.go:46:61: unnecessary use of fmt.Sprintf (S1039)

Fix CI error

ca7d545

https://github.com/cucumber/godog/runs/5146601108?check_suite_focus=true#step:7:28

Change CI to run staticcheck instead of golint

04aa1a4

carleeto force-pushed the switch-from-golint-to-staticcheck branch from d427b45 to 04aa1a4 Compare February 10, 2022 20:03

carleeto marked this pull request as draft February 10, 2022 20:08

carleeto changed the title ~~Switch from golint to staticcheck~~ WIP: Switch from golint to staticcheck Feb 10, 2022

Use staticcheck that definitely pass

dd60b1a

Fix CI staticcheck error

c0e5527

https://github.com/cucumber/godog/runs/5147133955?check_suite_focus=true#step:6:17

vearutop reviewed Feb 10, 2022

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

vearutop reviewed Feb 10, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

vearutop reviewed Feb 10, 2022

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

mattwynne and others added 3 commits February 10, 2022 13:03

Merge branch 'main' into switch-from-golint-to-staticcheck

129f122

Only run staticcheck for Go 1.17

9896794

Co-authored-by: Viacheslav Poturaev <nanopeni@gmail.com>

Add staticcheck linux binary

ed31677

carleeto added 2 commits February 12, 2022 10:52

Update Go module dependencies in _examples

d2b7c4b

Use static check binary in bin for CI

1523a48

carleeto and others added 2 commits February 12, 2022 11:12

Reduce number of dependencies

9c1d196

Also add a note to CONTRIBUTING.md about the _examples module

Pin the version of staticcheck

534ab13

Co-authored-by: Viacheslav Poturaev <nanopeni@gmail.com>

carleeto marked this pull request as ready for review February 12, 2022 02:49

carleeto changed the title ~~WIP: Switch from golint to staticcheck~~ Switch from golint to staticcheck Feb 12, 2022

Merge branch 'main' into switch-from-golint-to-staticcheck

8594a6c

vearutop approved these changes Feb 14, 2022

View reviewed changes

mattwynne added the 🏦 debt Tech debt label Feb 15, 2022

mattwynne approved these changes Feb 15, 2022

View reviewed changes

mattwynne merged commit c95871f into cucumber:main Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from golint to staticcheck #457

Switch from golint to staticcheck #457

carleeto commented Feb 10, 2022 •

edited

Loading

carleeto commented Feb 10, 2022

mattwynne commented Feb 10, 2022

vearutop commented Feb 10, 2022

mattwynne commented Feb 10, 2022

carleeto commented Feb 11, 2022 •

edited

Loading

codecov bot commented Feb 11, 2022 •

edited

Loading

carleeto commented Feb 11, 2022

mattwynne commented Feb 12, 2022 •

edited

Loading

carleeto commented Feb 12, 2022

mdelapenya commented Feb 12, 2022 •

edited

Loading

carleeto commented Feb 12, 2022 •

edited

Loading

carleeto commented Feb 13, 2022

mdelapenya commented Feb 13, 2022

mattwynne commented Feb 14, 2022

mattwynne commented Feb 14, 2022 •

edited

Loading

vearutop left a comment

vearutop commented Feb 14, 2022

mattwynne commented Feb 15, 2022

carleeto commented Feb 16, 2022

Switch from golint to staticcheck #457

Switch from golint to staticcheck #457

Conversation

carleeto commented Feb 10, 2022 • edited Loading

Description

Motivation & context

Type of change

Checklist:

carleeto commented Feb 10, 2022

mattwynne commented Feb 10, 2022

vearutop commented Feb 10, 2022

mattwynne commented Feb 10, 2022

carleeto commented Feb 11, 2022 • edited Loading

codecov bot commented Feb 11, 2022 • edited Loading

Codecov Report

carleeto commented Feb 11, 2022

mattwynne commented Feb 12, 2022 • edited Loading

carleeto commented Feb 12, 2022

mdelapenya commented Feb 12, 2022 • edited Loading

carleeto commented Feb 12, 2022 • edited Loading

carleeto commented Feb 13, 2022

mdelapenya commented Feb 13, 2022

mattwynne commented Feb 14, 2022

mattwynne commented Feb 14, 2022 • edited Loading

vearutop left a comment

Choose a reason for hiding this comment

vearutop commented Feb 14, 2022

mattwynne commented Feb 15, 2022

carleeto commented Feb 16, 2022

carleeto commented Feb 10, 2022 •

edited

Loading

carleeto commented Feb 11, 2022 •

edited

Loading

codecov bot commented Feb 11, 2022 •

edited

Loading

mattwynne commented Feb 12, 2022 •

edited

Loading

mdelapenya commented Feb 12, 2022 •

edited

Loading

carleeto commented Feb 12, 2022 •

edited

Loading

mattwynne commented Feb 14, 2022 •

edited

Loading