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(linter): fix golangci-lint warnings across substation #32

Merged
merged 3 commits into from
Oct 4, 2022

Conversation

shellcromancer
Copy link
Contributor

@shellcromancer shellcromancer commented Oct 3, 2022

This updates the Go code lintng to use the meta-linter golangci-lint and fixes related lints.

Description

Changes include:

  • fix: handle or explicitly ignore all errors
  • fix: remove redundant function calls (test: Update Tests to Use t.Errorf #31) and conversions
  • fix: rename predeclared caps identifier
  • chore: tools: gofmt -> gofumpt, and staticcheck -> golangci-lint
  • docs: update comments to remove grammar mistakes and follow Go comment conventions.

Motivation and Context

This closes #31, improves error handling and consistency across with project.

How Has This Been Tested?

Local builds, unit tests, and benchmarks all pass.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

* handle or explicitly ignore all errors
* remove redundant function calls/conversions
* rename predeclared `caps` identifier
* tools: gofmt -> gofumpt, and staticcheck -> golangci-lint
@shellcromancer shellcromancer marked this pull request as ready for review October 3, 2022 20:29
@shellcromancer shellcromancer requested a review from a team as a code owner October 3, 2022 20:29
if result.IsArray() {
return cap, fmt.Errorf("process pipeline: inputkey %s: %v", p.InputKey, errPipelineArrayInput)
return capsule, fmt.Errorf("process pipeline: inputkey %s: %v", p.InputKey, errPipelineArrayInput)
}

newCap := config.NewCapsule()
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be newCapsule to match the capsule convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point (and with newCapsules as well)! Modified these instances across the project with the latest fixup.

newCaps := newBatch(&caps)
for _, cap := range caps {
ok, err := op.Operate(ctx, cap)
newCaps := newBatch(&capsules)
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to the other comment, is newCapsules preferred since we're changing the convention to capsule?

process/copy.go Outdated
Comment on lines 14 to 19
JSON:
{"hello":"world"} >>> {"hello":"world","goodbye":"world"}
from JSON:
{"hello":"world"} >>> world
to JSON:
world >>> {"hello":"world"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

docs: this indent seems to have an extra tab compared to comments for other processors? is this correct? 🤔

Copy link
Contributor Author

@shellcromancer shellcromancer Oct 4, 2022

Choose a reason for hiding this comment

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

Whoops, no this was accidental - removed this extra indentation with the latest fixup. Thanks for catching this!

@jshlbrd
Copy link
Collaborator

jshlbrd commented Oct 3, 2022

fix: rename predeclared caps identifier

this feels like an opinionated change (which is OK), but we should document it so that others know that the naming convention is capsule or capsules and not cap or caps. could you add it to a new section in https://github.com/brexhq/substation/blob/main/CONTRIBUTING.md#naming-conventions?

Address code review comments.

Changes include:
- Undoing spurious whitespace changes
- Rename variables with modifiers related to `cap` like newCap
- Adding variable naming conventions to docs
@shellcromancer
Copy link
Contributor Author

this feels like an opinionated change (which is OK), but we should document it so that others know that the naming convention is capsule or capsules and not cap or caps. could you add it to a new section in https://github.com/brexhq/substation/blob/main/CONTRIBUTING.md#naming-conventions?

Added a new section to the contributions docs to touch on avoiding predeclared identifiers and how we chose to use capsule instead of caps.

@jshlbrd jshlbrd merged commit 9b7e077 into main Oct 4, 2022
@jshlbrd jshlbrd deleted the daniel/golangci-lint-fixes branch October 4, 2022 23:06
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.

test: Update Tests to Use t.Errorf
2 participants