Skip to content

update naturalsort sorting with slice - #7269

Merged
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:slice_and_dice_step2
Sep 3, 2026
Merged

thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:slice_and_dice_step2

Conversation

@thaJeztah

@thaJeztah thaJeztah commented Sep 2, 2026 •

Copy link
Copy Markdown
Member

vendor: github.com/fvbommel/sortorder v1.2.0

full diff: fvbommel/sortorder@v1.1.0...v1.2.0

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added this to the 29.8.0 milestone Sep 2, 2026
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 2, 2026
@codecov-commenter

codecov-commenter commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cli/cobra.go 0.00% 2 Missing ⚠️
cli/command/stack/services.go 0.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@thaJeztah
thaJeztah force-pushed the slice_and_dice_step2 branch from 4044fe5 to 6dc2fa8 Compare September 2, 2026 01:00

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Review incomplete

chunk 1: Drafter did not complete
chunk 2: Drafter did not complete
chunk 3: Drafter did not complete

All three drafter delegations failed with a sub-agent schema validation error (output_config.format.schema: Unsupported regex feature). The automated hypothesis-generation pipeline could not run.

Manual inspection summary (orchestrator-level): The diff was reviewed directly. This PR is a clean mechanical migration from sort.Slice/sort.Sort to slices.SortFunc/slices.SortStableFunc in tandem with vendoring github.com/fvbommel/sortorder v1.2.0 (which adds NaturalCompare for three-way comparison). All comparator semantics were verified to be preserved correctly:

  • sort.Slice → slices.SortFunc conversions: comparator directions correct throughout.
  • sort.SliceStable / sort.Stable → slices.SortStableFunc: stability preserved.
  • slices.SortedFunc(maps.Values(emptyMap), ...) returns nil in Go 1.24, so the updateUlimits "remove all" test expectation (nil) remains satisfied.
  • cmp.Or / cmp.Compare port-sort replacements match the original field ordering.
  • formatAdminRole key-ID sort now creates a copy instead of mutating the receiver — a bug fix.
  • toStringList %s → %v format change is safer for non-string any values.
  • filepath.IsLocal path-escape check in cli/config/config.go is more robust than the old strings.HasPrefix approach.

No bugs were identified in the manual pass. However, since the automated pipeline did not complete, this review cannot be treated as a full automated review.

Findings so far: ⚠️ INCOMPLETE

@thaJeztah
thaJeztah force-pushed the slice_and_dice_step2 branch from 6dc2fa8 to b7a8460 Compare September 2, 2026 07:47
@thaJeztah thaJeztah changed the title vendor: github.com/fvbommel/sortorder v1.2.0 and update sort update naturalsort sorting with slice Sep 2, 2026
@thaJeztah
thaJeztah force-pushed the slice_and_dice_step2 branch 4 times, most recently from 07c8f47 to 941e8e0 Compare September 3, 2026 12:57
@thaJeztah
thaJeztah marked this pull request as ready for review September 3, 2026 12:57
@thaJeztah
thaJeztah requested review from a team and silvin-lubecki as code owners September 3, 2026 12:57
@vvoland
vvoland requested a balanced review from Copilot September 3, 2026 12:58
vvoland
vvoland previously approved these changes Sep 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Top-level command sorting unintentionally loses its previous stability guarantee.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Migrates natural sorting from sort callbacks to slices APIs using sortorder v1.2.0.

Changes:

  • Replaces sort.Slice with slices.SortFunc.
  • Simplifies stable task sorting with cmp.Or.
  • Adds required Go 1.26 build constraints and concrete API imports.
File summaries
File Description
cmd/docker-trust/trust/inspect_pretty.go Migrates signer sorting.
cmd/docker-trust/trust/common.go Migrates signature-row sorting.
cli/context/store/metadatastore.go Migrates context metadata sorting.
cli/command/volume/list.go Migrates volume sorting.
cli/command/task/print.go Simplifies stable task sorting.
cli/command/system/prune.go Migrates prune-filter sorting.
cli/command/stack/services.go Migrates stack-service sorting.
cli/command/stack/list.go Migrates stack sorting.
cli/command/service/formatter.go Migrates service sorting.
cli/command/secret/ls.go Migrates secret sorting.
cli/command/plugin/list.go Migrates plugin sorting.
cli/command/node/list.go Migrates node sorting.
cli/command/network/list.go Migrates network sorting.
cli/command/context/list.go Migrates context sorting.
cli/command/container/port.go Migrates port-output sorting.
cli/command/config/ls.go Migrates config sorting.
cli/cobra.go Migrates top-command sorting.
cli-plugins/manager/manager.go Migrates CLI plugin sorting.
Review details
  • Files reviewed: 18/18 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cli/cobra.go Outdated
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztah merged commit b1b8a60 into docker:master Sep 3, 2026
95 checks passed
@thaJeztah
thaJeztah deleted the slice_and_dice_step2 branch September 3, 2026 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants