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

Run golint and go vet #539

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Run golint and go vet #539

wants to merge 2 commits into from

Conversation

Madrigal
Copy link
Contributor

Issue #, if available:

Description of changes:
Ran golint and make vet on the repo.

We had an offline discussion about running this always. golint is unfortunately deprecated, and there's no easy way to tell the tool "ignore this module/file"

  • Using find -exec golint doesn't work as expected since golint expects to be run in the context of a package
  • running golint ./... marks singleflight as failing check, so we can't use the exit code to accept/reject a change

Looked into incorporating golangci-lint, but the scope of changes it flags doesn't seem too valuable, so I'm a bit hesitant to add it to the default run if we're just going to ignore the result.

golint flagged these changes, which I

golint   ./...
encoding/doc.go:3:1: package comment is detached; there should be no blank lines between it and the package statement
encoding/cbor/encode.go:192:1: comment on exported type EncodeFixedUint should be of the form "EncodeFixedUint ..." (with optional leading article)
encoding/cbor/encode.go:206:1: comment on exported type EncodeFixedNegInt should be of the form "EncodeFixedNegInt ..." (with optional leading article)
encoding/httpbinding/encode.go:36:1: comment on exported function NewEncoderWithRawPath should be of the form "NewEncoderWithRawPath ..."
endpoints/private/rulesfn/doc.go:3:1: package comment is detached; there should be no blank lines between it and the package statement
endpoints/private/rulesfn/strings.go:3:1: comment on exported function SubString should be of the form "SubString ..."
endpoints/private/rulesfn/uri.go:85:2: struct field IsIp should be IsIP
internal/sync/singleflight/docs.go:7:1: package comment is detached; there should be no blank lines between it and the package statement
internal/sync/singleflight/singleflight.go:84:1: error should be the last type when returning multiple items (NOTE this is the one we don't want to fix, since it will break compatibility with the published package)
middleware/stack_values.go:24:1: comment on exported function GetStackValue should be of the form "GetStackValue ..."
middleware/stack_values.go:65:1: receiver name c should be consistent with previous receiver name m for stackValues
private/requestcompression/request_compression.go:18:7: exported const MaxRequestMinCompressSizeBytes should have comment or be unexported
testing/gzip.go:10:1: exported function GzipCompareCompressBytes should have comment or be unexported
testing/xml/doc.go:1:1: package comment should be of the form "Package xml ..."
testing/xml/xmlToStruct.go:12:6: type name will be used as xml.XMLNode by other packages, and that stutters; consider calling this Node
testing/xml/xmlToStruct.go:42:6: func name will be used as xml.XMLToStruct by other packages, and that stutters; consider calling this ToStruct
transport/http/headerlist.go:58:8: should replace j += 1 with j++
transport/http/properties.go:49:1: comment on exported function GetSigV4ASigningRegions should be of the form "GetSigV4ASigningRegions ..."

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Madrigal Madrigal requested review from a team as code owners September 18, 2024 21:07
@Madrigal
Copy link
Contributor Author

API diff failed with

# github.com/aws/smithy-go/endpoints/private/rulesfn
## incompatible changes
URL.IsIp: removed
## compatible changes
URL.IsIP: added

# github.com/aws/smithy-go/testing
## incompatible changes
GzipCompareCompressBytes: removed

# github.com/aws/smithy-go/testing/xml
## incompatible changes
XMLNode: removed
XMLToStruct: removed
## compatible changes
Node: added
ToStruct: added

I thought these changes were private to the package and not exposed outside, @lucix-aws are they breaking changes?

@Madrigal
Copy link
Contributor Author

Madrigal commented Sep 18, 2024

Discussed offline, these changes are indeed not backwards compatible, this is an example where we were bitten by this in the past.

Will revert those changes

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.

1 participant