Update build tooling from starter template and fix linter issues #964
Conversation
- Update Go version from 1.24.6 to 1.24.11
- Update golangci-lint to v2.8.0 with new config format
- Update gotestsum to v1.13.0
- Update Node.js version from 16.13.1 to 20.11
- Add manifest-check target to Makefile
- Use Go 1.24 features (strings.SplitSeq, slices.Contains)
- Replace interface{} with any throughout codebase
- Replace custom contains functions with slices.Contains
- Convert if-else chains to tagged switches
- Simplify embedded field selectors (c.Context.Ctx → c.Ctx)
- Use strings.Builder for efficient string concatenation
- Apply De Morgan's law simplifications
- Fix directory permissions (0755 → 0750)
- Add license headers to build files
Co-Authored-By: Claude Opus 4.5 <[email protected]>
nevyangelova
left a comment
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @hanzei some minor changes. Also I see tests for contains and SliceContainsString were removed but the tests for containsValue in utils_test.go were also removed. Consider keeping at least one integration test to verify the behavior with your specific use cases.
server/plugin/api.go
Outdated
| // Only send down fields to client that are needed | ||
| type RepositoryResponse struct { | ||
| DefaultRepo RepoResponse `json:"defaultRepo,omitempty"` | ||
| DefaultRepo RepoResponse `json:"defaultRepo"` |
There was a problem hiding this comment.
Is there a reason for removing omitempty? it causes an empty {} to be sent when no default repo is set. The frontend checks if (yourReposByOrg?.defaultRepo) which is truthy for {} causing undefined values to be passed to onChangeForRepo().
There was a problem hiding this comment.
Good question. The original code has a bug. omitempty was never working as intended. For struct types, omitempty never considers the fields as empty.
I've changed the code to use omitzero instead, which works for structs.
|
|
||
| orgs := strings.Split(configuredOrgs, ",") | ||
| for _, org := range orgs { | ||
| for org := range strings.SplitSeq(configuredOrgs, ",") { |
There was a problem hiding this comment.
Those are Go 1.24+ features. we should ensure all CI/CD pipelines and developer environments support them
There was a problem hiding this comment.
Per the go.mod file, go 1.24 was already used before this PR. Is there anything else we need to check?
| @@ -1635,7 +1600,6 @@ func TestHandleSubscribe(t *testing.T) { | |||
| name: "default case, handleSubscribesAdd called", | |||
| parameters: []string{"invalid_parameter_1", "invalid_parameter_2", "invalid_parameter_3"}, | |||
| setup: func() { | |||
There was a problem hiding this comment.
is this empty function needed?
There was a problem hiding this comment.
The test expects it:
Do we need to change that?
Co-Authored-By: Claude Opus 4.6 <[email protected]>
omitempty has no effect on non-pointer struct fields in JSON encoding. Go 1.24's omitzero correctly omits the field when the struct is its zero value. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The code now uses |
2 participants
Summary
make check-styleChanges
Build Tooling
.golangci.ymlto version 2 format with new linters (bidichk, makezero, modernize, unqueryvet)Makefilewith new tool versions and manifest-check targetbuild/setup.mkbuild/manifest/main.goCode Modernization
interface{}withanythroughout codebasecontains/SliceContainsStringfunctions withslices.Containsstrings.SplitSeq,fmt.Appendf)c.Context.Ctx→c.Ctx)strings.Builderfor efficient string concatenation in loopsConfig Files
.nvmrcfrom 16.13.1 to 20.11.editorconfig.gitignoreTest plan
make check-stylepasses with 0 issuesmake testpasses (435 Go tests + 22 webapp tests)🤖 Generated with Claude Code