Fix/modernize codebase#524
Conversation
…max builtin, remove omitempty - acp: replace manual contains loop with slices.Contains, use for range N - background: remove omitempty from non-pointer CompletedAt field - agenteval: remove omitempty from value-type ContextChecks field, use SplitSeq - config: use slices.Contains, max builtin for clamp patterns
- cloneArgs: replace manual map copy loop with maps.Copy - containsToolName: replace manual loop with slices.Contains - containsPermissionDecision test helper: replace manual loop with slices.Contains
…+slice - command_prefix.go: use CutSuffix instead of HasSuffix+TrimSuffix, use SplitSeq - guardrails.go: use strings.Cut instead of Index+slice in IsNoProgressStop - compaction_preserve.go: use strings.Cut instead of IndexByte for heading extraction - system_prompt.go: use CutPrefix instead of HasPrefix+TrimPrefix for git branch
- compaction.go: use max() instead of manual boundary clamp - compaction_preserve.go: use max() for capBody limit clamp - system_prompt.go: use max() for truncateGuidelineContent clamp
- Replace for i := 0; i < N; i++ with for range N or for i := range N - Covers compaction, defer_savings, guardrails, reconnect, skills_context tests
- agent_eval, app_test, exec_spec: use slices.Contains for manual contains loops - cron_claim_test: use for range N with wg.Go instead of manual wg.Add+go - cron_run: use SplitSeq, slices.Contains for contains() - daemon: use strings.Cut for flag parsing - deferred_wiring_test, sandbox_test: use for range N - exec_tools, mcp_config, provider_setup: use slices.Contains - provider_models, provider_onboarding: use max builtin - usage_test: remove unused import
…SplitSeq, for range N, min/max - contextreport: use slices.Contains for contains patterns, for range N - cron: use for range N, strings.Cut instead of IndexByte - daemon: use for range N, slices.Contains, for range for session_race_test - hooks: use slices.Contains, for range N with wg.Go - imageinput: use max(), strings.Cut for clipboard - installtest: use for range N - keyring: use slices.Contains - localcontrol: use for range N - lsp: use for range N, slices.Contains
- mcp: simplify slices.Contains, for range N patterns - oauth: use slices.Contains, strings.Cut, for range N, maps.Copy instead of manual copy loops
…t, max builtin - models.go: replace manual slices with slices.Contains, use maps.Keys for JSON tags - cost.go: use max/min builtins for clamp, use slices.Contains - resolve.go: use strings.Cut instead of HasPrefix+TrimPrefix+cutset - catalog_test.go: use for range N
…builtin - gemini/provider.go: use strings.CutPrefix - gemini/types.go: use slices.Contains for contains - openai/provider.go: use max() clamp - openai/provider_test.go: use for range N - providerio: use slices.Contains, SplitSeq, max, for range N for retry
…r range N, slices.Contains, SplitSeq - sessions: use for range N, for range in tests with wg.Go, strings.Cut, max builtin - notify: use for range N - perfbench: use max(), slices.Contains - plugins: use for range N - redaction: use for range N for test loops - release: use for range N
… maps.Copy, for range N - repoinfo: use slices.Contains, maps.Copy instead of manual loops - review: use strings.Cut - securefile: use slices.Contains - selfverify: use for range N
…range N - skills, specialist: use slices.Contains, for range N - specmode: use slices.Contains - swarm: use for range N in tests
…, simplify edit_replacers - apply_patch: use strings.Cut instead of IndexByte - bash: use strings.Cut - edit_replacers: replace manual loop with slices.Contains, use maps.Invert, use strconv.Atoi directly - exec_command_test: use for range N, wg.Go - output_budget: use slices.Contains - tool_search: use strings.Cut, slices.Contains - Remove unused import in scaffold_test
…itSeq in views and panels - rendering.go: use max/min builtins for clamp patterns, strings.Cut instead of Index+slice - view.go: use strings.CutPrefix for git branch parsing, use SplitSeq for diff detection - sidebar.go: use max(), strings.FieldsSeq, use slices.Contains - file_view.go, files_panel.go: use max(), slices.Contains - startup.go: use max() for padding calculation - model.go: use slices.Contains, maps.Keys for response style detection - command_center, commands, composer: use slices.Contains - onboarding: use max(), slices.Contains - ripple: use slices.Contains, max() clamp - plan_panel, plan_step_detail: use slices.Contains, SplitSeq - specialist_card: use max() for clamp patterns - session_controls: use slices.Contains - keybinding_help: use slices.Contains, maps.Keys for sorting - files_git_sweep: use slices.Contains - assistant_markdown: use SplitSeq, slices.Contains - pr_status: use slices.Contains - streaming_fade: use max() for age calculation - streaming_decoder: use for range N
…ts and remaining files - Use for range N for test iteration loops - Use strings.CutPrefix instead of HasPrefix+TrimPrefix - Use strings.SplitSeq instead of strings.Split - Use slices.Contains for contains checks - session_title.go: use SplitSeq for title cleaning
…SplitSeq, slices.Contains - update: use strings.Cut instead of Index+slice for comma parsing - usercommands: use SplitSeq for frontmatter iteration - verify: use SplitSeq for output summarization, slices.Contains for test - workspaceseed: use slices.Contains for memoryFiles detection
- zerogit diffstat: use SplitSeq instead of Split - zerogit zerogit.go: use SplitSeq, min() for truncation - zeroruntime types.go: use strings.Cut instead of IndexByte for media type parsing
|
if you guys want to fix the but for that i need some info. |
kevincodex1
requested review from
anandh8x and
gnanam1990
and removed request for
gnanam1990
|
Thanks for this — modernizing to the newer Go idioms ( What's holding me back isn't the idioms, it's the packaging. This is 187 files across nearly every package, and a sweep that size is hard to take as one PR for a few reasons:
What I'd suggest instead: split it per package (or per idiom) into small, independently reviewable PRs, and ideally land them mechanically — Not a no to the idea — it's a genuinely good direction, it just needs to come in as small verifiable pieces rather than one big sweep. If you want to start with a single package as a template, I'm happy to review that quickly and we can pattern the rest off it. (One process note: #523 is really a placeholder — this isn't a bug, so it doesn't fit the issue-first flow. If we go the split-PR route, a short "modernize to Go 1.22 idioms" tracked task with the plan would be the cleaner parent.) |
|
this is massive :) |
|
Thank you for the contribution. I appreciate the time and effort you put into trying to improve the project. For now, I’m going to close this because the PR is much broader than we can review or accept in one pass. It touches a large number of files and appears to include general modernization/refactoring work rather than a small, approved, issue-scoped change. Please see
This PR falls into that category because it is a broad modernization/refactor across many files rather than a narrowly scoped fix tied to an approved issue. Smaller, focused PRs are much easier for us to evaluate, test, and merge safely. If there is a specific bug or functional improvement you want to propose, the best next step is to open or comment on an issue first, keep the scope small, and wait for it to be marked with Thanks again for wanting to help. |
|
thanks everyone for the feedback that makes sense. ill split this into smaller, issue-scoped PRs (likely by package/idiom) after opening an approved tracking issue, rather than submitting one large modernization sweep. Appreciate the guidance. |
4 participants
Summary
this is just modernizing the codebase like the std lib upgrades and stuff.
Linked issue
Fixes #523
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).