Add linting with golangci-lint #722

jamietanna · 2022-08-16T20:05:56Z

No description provided.

jamietanna · 2022-10-25T09:29:53Z

@deepmap-marcinr mind having a look? I need to clean up the commit history (hence this still being a draft) but would be good to get this through to start giving us a bit more control + confidence in our code style, as well as closing #58.

jamietanna · 2022-10-25T09:30:50Z

pkg/gin-middleware/oapi_validate.go

@@ -122,12 +122,12 @@ func ValidateRequestFromContext(c *gin.Context, router routers.Router, options *

 	// Pass the gin context into the request validator, so that any callbacks
 	// which it invokes make it available.
-	requestContext := context.WithValue(context.Background(), GinContextKey, c)
+	requestContext := context.WithValue(context.Background(), GinContextKey, c) //nolint:staticcheck


SA1029: should not use built-in type string as key for value; define your own type to avoid collisions (staticcheck)

jamietanna · 2022-10-25T09:31:41Z

pkg/runtime/bindform.go

@@ -99,7 +99,7 @@ func MarshalForm(ptr interface{}, encodings map[string]RequestBodyEncoding) (url
 		tag = strings.Split(tag, ",")[0] // extract the name of the tag
 		if encoding, ok := encodings[tag]; ok && encoding.ContentType != "" {
 			if strings.HasPrefix(encoding.ContentType, jsonContentType) {
-				if data, err := json.Marshal(field); err != nil {
+				if data, err := json.Marshal(field); err != nil { //nolint:staticcheck


SA9005: struct type 'reflect.Value' doesn't have any exported fields, nor custom marshaling

jamietanna · 2022-10-25T09:32:04Z

pkg/runtime/deepobject.go

@@ -242,7 +242,7 @@ func assignPathValues(dst interface{}, pathValues fieldOrValue) error {
 			tm, err = time.Parse(time.RFC3339Nano, pathValues.value)
 			if err != nil {
 				// Fall back to parsing it as a date.
-				tm, err = time.Parse(types.DateFormat, pathValues.value)
+				tm, err = time.Parse(types.DateFormat, pathValues.value) //nolint:ineffassign,staticcheck


ineffectual assignment to tm (ineffassign)

deepmap-marcinr · 2022-10-28T00:12:20Z

I rebased your branch, and fixed up a bunch of merge conflicts, however, you're making breaking API changes due to initialisms - people depending on the testutils or runtime packages will see code break. Our hands are kinda tied there until a breaking release.

jamietanna · 2022-10-30T08:34:08Z

I rebased your branch, and fixed up a bunch of merge conflicts, however, you're making breaking API changes due to initialisms - people depending on the testutils or runtime packages will see code break. Our hands are kinda tied there until a breaking release.

Ah that's fair, I'll ignore the initialisms check, for now then 👍

stevenh · 2022-11-11T02:39:52Z

Hi @jamietanna I added golangci-lint separately in #845 see if it does what you need it to?

Right now, we can ignore the return values, as we're hardcoding the response of an `err` to always return, which isn't ideal.

As we've added them on purpose, to show that they don't get (un)marshalled.

As it's probably a bit late to change this, in case it changes expected behaviour, via SA1029.

Via S1007

Via SA1019

Or, least show that we've considered whether to ignore them, which we do to follow existing style of the file to ignore everything.

Via S1028

As we'll come back to it later. pkg/runtime/deepobject.go:245:5: ineffectual assignment to tm (ineffassign) tm, err = time.Parse(types.DateFormat, pathValues.value)

pkg/runtime/bindform.go:102:34: SA9005: struct type 'reflect.Value' doesn't have any exported fields, nor custom marshaling (staticcheck) if data, err := json.Marshal(field); err != nil {

To give a bit more consistency across the project with the Go ecosystem, as well as picking up on some common errors that new contributions can bring, enable golangci-lint, with default configuration.

stevenh

Few nits but the the main issue is just using _ = function() to quell error not checked, we should never do that.

stevenh · 2022-11-12T23:36:06Z

internal/test/issues/issue-312/issue_test.go

@@ -26,7 +26,8 @@ func TestClient_WhenPathHasColon_RequestHasCorrectPath(t *testing.T) {
 		assert.Equal(t, "http://host/pets:validate", req.URL.String())
 	})

-	client.ValidatePetsWithResponse(context.Background(), ValidatePetsJSONRequestBody{
+	// TODO: this err should be checked, but it defaults to returning the "something went wrong", so we should refactor accordingly


We should just check the errors, its easy enough, also check the errors for NewClient above

stevenh · 2022-11-12T23:37:03Z

pkg/codegen/prune.go

@@ -28,14 +28,14 @@ func walkSwagger(swagger *openapi3.T, doFn func(RefWrapper) (bool, error)) error

 	for _, p := range swagger.Paths {
 		for _, param := range p.Parameters {
-			walkParameterRef(param, doFn)
+			_ = walkParameterRef(param, doFn)


You should never assign to _ just to silence a lint error, either check the error if its genuine or add a //nolint: errcheck comment to the line when its not such as a .Close() on a io.ReadCloser(), lots more below.

Although yes, I 100% agree, the file so far has a lot of these, and I'd prefer us to come back and sort them

I see a two options:

Disable errcheck linter in .golangci-lint.yaml for now.

Use ci: enable golangci-lint and fix errors #845 which already fixes all of these.

stevenh · 2022-11-23T20:55:47Z

Great to see this merged!

I've rebased my golangci-lint changes on top of it, hopefully we can get that in too.

Add linting with golangci-lint

leonnicolas mentioned this pull request Aug 29, 2022

Remove io/ioutil #727

Merged

jamietanna mentioned this pull request Sep 11, 2022

golint compliance #58

Closed

jamietanna force-pushed the chore/golangci-lint branch from 0da0d80 to b04a972 Compare October 25, 2022 08:00

jamietanna commented Oct 25, 2022

View reviewed changes

jamietanna requested a review from deepmap-marcinr October 25, 2022 09:32

deepmap-marcinr force-pushed the chore/golangci-lint branch from 8cec783 to dc33c3c Compare October 28, 2022 00:08

jamietanna force-pushed the chore/golangci-lint branch 2 times, most recently from 1c3f13d to 677718e Compare October 30, 2022 08:31

stevenh mentioned this pull request Nov 10, 2022

ci: enable golangci-lint and fix errors #845

Closed

Jamie Tanna added 9 commits November 12, 2022 21:04

Lint: remove unused variable

c9ba994

Lint: Check return values

afcb5eb

Right now, we can ignore the return values, as we're hardcoding the response of an `err` to always return, which isn't ideal.

Lint: Remove unused unexported variable

b87ee6a

Lint: Use consistent Go-style package names

d94bf5c

Lint: unconvert

cb7bd58

Lint: Exclude unexported fields for structcheck

8079cb1

As we've added them on purpose, to show that they don't get (un)marshalled.

Lint: Ignore findings on string-based keys for Context

94fc374

As it's probably a bit late to change this, in case it changes expected behaviour, via SA1029.

Lint: Use backtick'd strings for regexes

e9cf29e

Via S1007

Lint: Don't use deprecated strings.Title

f3a2d72

Via SA1019

jamietanna force-pushed the chore/golangci-lint branch from f36b5a7 to f92f881 Compare November 12, 2022 21:05

Jamie Tanna added 5 commits November 12, 2022 21:06

Lint: remove ineffectual assignments

4202f53

Lint: Ensure we check errors

a708e69

Or, least show that we've considered whether to ignore them, which we do to follow existing style of the file to ignore everything.

Lint: Use fmt.Errorf instead of errors.New(fmt.Sprintf)

43953e7

Via S1028

Lint: remove warning for ineffectual

895c8ac

As we'll come back to it later. pkg/runtime/deepobject.go:245:5: ineffectual assignment to tm (ineffassign) tm, err = time.Parse(types.DateFormat, pathValues.value)

Lint: Simplify make for a slice

7efd543

Jamie Tanna added 2 commits November 12, 2022 21:06

Lint: Ignore marshalling warning with reflect.Value

0ee106e

pkg/runtime/bindform.go:102:34: SA9005: struct type 'reflect.Value' doesn't have any exported fields, nor custom marshaling (staticcheck) if data, err := json.Marshal(field); err != nil {

Add linting with golangci-lint

5a103c5

To give a bit more consistency across the project with the Go ecosystem, as well as picking up on some common errors that new contributions can bring, enable golangci-lint, with default configuration.

jamietanna force-pushed the chore/golangci-lint branch from f92f881 to 5a103c5 Compare November 12, 2022 21:06

jamietanna marked this pull request as ready for review November 12, 2022 21:08

jamietanna mentioned this pull request Nov 12, 2022

feat(pkg/codegen): update for common initialisms #13

Closed

stevenh suggested changes Nov 12, 2022

View reviewed changes

jamietanna assigned deepmap-marcinr Nov 13, 2022

jamietanna merged commit 40890b2 into deepmap:master Nov 23, 2022

jamietanna deleted the chore/golangci-lint branch November 23, 2022 09:09

adrianpk pushed a commit to foorester/oapi-codegen that referenced this pull request Jan 16, 2024

Merge pull request deepmap#722 from jamietanna/chore/golangci-lint

b40b341

Add linting with golangci-lint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add linting with golangci-lint #722

Add linting with golangci-lint #722

jamietanna commented Aug 16, 2022 •

edited

jamietanna commented Oct 25, 2022

jamietanna Oct 25, 2022 •

edited

jamietanna Oct 25, 2022

jamietanna Oct 25, 2022

deepmap-marcinr commented Oct 28, 2022

jamietanna commented Oct 30, 2022

stevenh commented Nov 11, 2022

stevenh left a comment

stevenh Nov 12, 2022

stevenh Nov 12, 2022 •

edited

jamietanna Nov 13, 2022

stevenh Nov 13, 2022

stevenh commented Nov 23, 2022

Add linting with golangci-lint #722

Add linting with golangci-lint #722

Conversation

jamietanna commented Aug 16, 2022 • edited

jamietanna commented Oct 25, 2022

jamietanna Oct 25, 2022 • edited

Choose a reason for hiding this comment

jamietanna Oct 25, 2022

Choose a reason for hiding this comment

jamietanna Oct 25, 2022

Choose a reason for hiding this comment

deepmap-marcinr commented Oct 28, 2022

jamietanna commented Oct 30, 2022

stevenh commented Nov 11, 2022

stevenh left a comment

Choose a reason for hiding this comment

stevenh Nov 12, 2022

Choose a reason for hiding this comment

stevenh Nov 12, 2022 • edited

Choose a reason for hiding this comment

jamietanna Nov 13, 2022

Choose a reason for hiding this comment

stevenh Nov 13, 2022

Choose a reason for hiding this comment

stevenh commented Nov 23, 2022

jamietanna commented Aug 16, 2022 •

edited

jamietanna Oct 25, 2022 •

edited

stevenh Nov 12, 2022 •

edited