Repository navigation
fix(themehost): keep the active theme first and sort the rest naturally #515
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package themehost_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/gopherium/gophenberg/internal/themehost" | ||
| ) | ||
|
|
||
| // WordPress keeps the active theme on top and sorts the rest with case ignored | ||
| // and numbers in natural order. Sorting by raw bytes put a capitalised or | ||
| // accented name out of place and read theme10 before theme2 (#509). | ||
| func TestManagerListPutsTheActiveThemeFirstAndSortsTheRestNaturally(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| settings := newSettings() | ||
| settings.values[themehost.ActiveKey] = "theme2" | ||
| manager := managerOver(t, settings, "", "Zoe", "theme10", "alba", "Álvaro", "theme2") | ||
|
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: internal/themehost/manager_list_order_test.go
Line: 19-20
Comment:
**Numeric sorting goes unchecked**
`theme2` is active, so the active-first check puts it before `theme10` without comparing their names. A case-insensitive alphabetical comparison would still pass this test, leaving numeric sorting regressions undetected. Add a case where both numbered themes are inactive and assert that `theme2` comes before `theme10`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
|
|
||
| installed, err := manager.List(context.Background()) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| got := make([]string, len(installed)) | ||
| for i, theme := range installed { | ||
| got[i] = theme.Name | ||
| } | ||
| want := []string{"theme2", "alba", "theme10", "Zoe", "Álvaro"} | ||
| if len(got) != len(want) { | ||
| t.Fatalf("listed %d themes, want %d: %v", len(got), len(want), got) | ||
| } | ||
| for i := range want { | ||
| if got[i] != want[i] { | ||
| t.Fatalf("theme order = %v, want %v", got, want) | ||
| } | ||
| } | ||
| if !installed[0].Active { | ||
| t.Fatalf("the first theme is %q, not the active one", installed[0].Name) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,97 @@ | ||||||||||||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||
|
|
||||||||||||||||
| package themehost | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "sort" | ||||||||||||||||
| "unicode" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| // ordered puts the active theme first and sorts the rest by name with case | ||||||||||||||||
| // ignored and digit runs compared by value. WordPress builds its themes screen | ||||||||||||||||
| // with wp_prepare_themes_for_js, which keeps the active theme on top and sorts | ||||||||||||||||
| // the rest with WP_Theme::sort_by_name; raw byte order moved a capitalised or | ||||||||||||||||
| // accented name out of place and read theme10 before theme2 (#509). | ||||||||||||||||
|
Comment on lines
+10
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Make the Lines 11-14 explain why the function exists. They mention 📝 Proposed fix--- "a/internal/themehost/order.go"
+++ "b/internal/themehost/order.go"
@@ -7,11 +7,8 @@
"unicode"
)
-// ordered puts the active theme first and sorts the rest by name with case
-// ignored and digit runs compared by value. WordPress builds its themes screen
-// with wp_prepare_themes_for_js, which keeps the active theme on top and sorts
-// the rest with WP_Theme::sort_by_name; raw byte order moved a capitalised or
-// accented name out of place and read theme10 before theme2 (#509).
+// ordered puts the active theme first and sorts the rest by name with case
+// ignored and digit runs compared by value.
func ordered(themes []Installed) []Installed {
sort.SliceStable(themes, func(i, j int) bool {
if themes[i].Active != themes[j].Active {As per path instructions: "A docblock starts with the name of what it documents, and states WHAT that thing does and never how or why." 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||
| func ordered(themes []Installed) []Installed { | ||||||||||||||||
| sort.SliceStable(themes, func(i, j int) bool { | ||||||||||||||||
| if themes[i].Active != themes[j].Active { | ||||||||||||||||
| return themes[i].Active | ||||||||||||||||
| } | ||||||||||||||||
| return naturalLess(themes[i].Name, themes[j].Name) | ||||||||||||||||
| }) | ||||||||||||||||
| return themes | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // naturalLess compares two names the way PHP's strnatcasecmp does: letters | ||||||||||||||||
| // case-insensitively, and each run of digits by its numeric value so theme2 | ||||||||||||||||
| // comes before theme10. | ||||||||||||||||
|
Comment on lines
+25
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove the The phrase "the way PHP's strnatcasecmp does" describes how the function relates to PHP. It does not say what the function does. The comparison is also not exact. 📝 Proposed fix--- "a/internal/themehost/order.go"
+++ "b/internal/themehost/order.go"
@@ -22,9 +22,8 @@
return themes
}
-// naturalLess compares two names the way PHP's strnatcasecmp does: letters
-// case-insensitively, and each run of digits by its numeric value so theme2
-// comes before theme10.
+// naturalLess reports whether a sorts before b, comparing letters
+// case-insensitively and each run of digits by its numeric value.
func naturalLess(a, b string) bool { return naturalCompare(a, b) < 0 }
func naturalCompare(a, b string) int {As per path instructions: "states WHAT that thing does and never how or why." 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||
| func naturalLess(a, b string) bool { return naturalCompare(a, b) < 0 } | ||||||||||||||||
|
|
||||||||||||||||
| func naturalCompare(a, b string) int { | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: Helpers in order.go lack the doc comments CONTRIBUTING requiresCONTRIBUTING says every function needs a doc comment in canonical Go form, and Test functions are the one exception that should have none. This PR gets both sides wrong. Add canonical doc comments to the helpers. Separately, remove the doc comment above the Test function.: Was this helpful? React with 👍 / 👎 |
||||||||||||||||
| ar, br := []rune(a), []rune(b) | ||||||||||||||||
| i, j := 0, 0 | ||||||||||||||||
| for i < len(ar) && j < len(br) { | ||||||||||||||||
| if isDigit(ar[i]) && isDigit(br[j]) { | ||||||||||||||||
| ai, bj := i, j | ||||||||||||||||
| for ai < len(ar) && isDigit(ar[ai]) { | ||||||||||||||||
| ai++ | ||||||||||||||||
| } | ||||||||||||||||
| for bj < len(br) && isDigit(br[bj]) { | ||||||||||||||||
| bj++ | ||||||||||||||||
| } | ||||||||||||||||
| if c := compareDigits(string(ar[i:ai]), string(br[j:bj])); c != 0 { | ||||||||||||||||
| return c | ||||||||||||||||
| } | ||||||||||||||||
| i, j = ai, bj | ||||||||||||||||
| continue | ||||||||||||||||
| } | ||||||||||||||||
| al, bl := unicode.ToLower(ar[i]), unicode.ToLower(br[j]) | ||||||||||||||||
| if al != bl { | ||||||||||||||||
| if al < bl { | ||||||||||||||||
| return -1 | ||||||||||||||||
| } | ||||||||||||||||
| return 1 | ||||||||||||||||
| } | ||||||||||||||||
| i++ | ||||||||||||||||
| j++ | ||||||||||||||||
| } | ||||||||||||||||
| switch { | ||||||||||||||||
| case i < len(ar): | ||||||||||||||||
| return 1 | ||||||||||||||||
| case j < len(br): | ||||||||||||||||
| return -1 | ||||||||||||||||
| } | ||||||||||||||||
| return 0 | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+30
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ New issue: Complex Method
Comment on lines
+30
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ New issue: Bumpy Road Ahead |
||||||||||||||||
|
|
||||||||||||||||
| func compareDigits(a, b string) int { | ||||||||||||||||
| at, bt := trimLeadingZeros(a), trimLeadingZeros(b) | ||||||||||||||||
| if len(at) != len(bt) { | ||||||||||||||||
| if len(at) < len(bt) { | ||||||||||||||||
| return -1 | ||||||||||||||||
| } | ||||||||||||||||
| return 1 | ||||||||||||||||
| } | ||||||||||||||||
| if at != bt { | ||||||||||||||||
| if at < bt { | ||||||||||||||||
| return -1 | ||||||||||||||||
| } | ||||||||||||||||
| return 1 | ||||||||||||||||
| } | ||||||||||||||||
| if len(a) != len(b) { | ||||||||||||||||
| if len(a) < len(b) { | ||||||||||||||||
| return -1 | ||||||||||||||||
| } | ||||||||||||||||
| return 1 | ||||||||||||||||
| } | ||||||||||||||||
| return 0 | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+67
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ New issue: Bumpy Road Ahead |
||||||||||||||||
|
|
||||||||||||||||
| func trimLeadingZeros(s string) string { | ||||||||||||||||
| for len(s) > 1 && s[0] == '0' { | ||||||||||||||||
| s = s[1:] | ||||||||||||||||
| } | ||||||||||||||||
| return s | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func isDigit(r rune) bool { return r >= '0' && r <= '9' } | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the doc comment above
TestManagerListPutsTheActiveThemeFirstAndSortsTheRestNaturally.Test functions in this repository carry no doc comment. The test name already states what it checks. This comment also explains why (WordPress behavior, issue
#509) and does not start with the declaration name.📝 Proposed fix
As per path instructions: "Test functions here deliberately carry no doc comment."
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions