Skip to content

[review-factory test] textutil: keep acronyms together in CamelToSnakeCase #7633

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.

Sign up for GitHub

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

Closed
Sankalp-Mittal wants to merge 1 commit into from
+27 −3
Closed

[review-factory test] textutil: keep acronyms together in CamelToSnakeCase #7633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions libs/textutil/case.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,19 @@ package textutil

import "unicode"

// CamelToSnakeCase converts camelCase or PascalCase to snake_case. A run of

@Sankalp-Mittal Sankalp-Mittal Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

minor: Doc comment overstates acronym handling: a run of capitals followed by a lowercase letter still splits (userIDs -> user_i_ds)

The doc comment says "A run of capitals is kept together as one word". That only holds when the run is followed by the end of the string, a digit, or another non-lowercase rune. When a lowercase rune follows the run, the last capital starts a new word (unicode.IsUpper(prev) && nextIsLower at case.go:15). This is what gives HTTPServer -> http_server, but it also splits plural acronyms and mixed-case names: userIDs -> user_i_ds, IDs -> i_ds, getURLs -> get_ur_ls, MLflow -> m_lflow, IPv4Address -> i_pv4_address. The two-capital outputs are the same as before this PR, so this is not a regression, but a reader of the comment (and of the PR title) would expect user_ids.

Evidence: hand traces and a line-for-line Python port of the old and new bodies (no Go toolchain was available, so the Go test was not run). No entry in case_test.go:14-49 has an acronym followed by lowercase other than a full following word.

Suggestion: state the rule as implemented (the last capital of a run begins the next word when a lowercase letter follows) and add a table entry such as userIDs so the limitation is pinned deliberately. The reviewers do not think the algorithm needs to change, since plurals are ambiguous with HTTPServer.

Agreed by 2 reviewer model(s).

// capitals is kept together as one word, so "HTTPServer" becomes "http_server"
// rather than "h_t_t_p_server".
func CamelToSnakeCase(name string) string {
out := make([]rune, 0, len(name)*2)
for i, r := range name {
runes := []rune(name)
out := make([]rune, 0, len(runes)*2)
for i, r := range runes {
if i > 0 && unicode.IsUpper(r) {
out = append(out, '_')
prev := runes[i-1]
nextIsLower := i+1 < len(runes) && unicode.IsLower(runes[i+1])
if unicode.IsLower(prev) || unicode.IsDigit(prev) || (unicode.IsUpper(prev) && nextIsLower) {

@Sankalp-Mittal Sankalp-Mittal Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

minor: Underscore is also dropped after any previous rune that is not lowercase, a digit, or an uppercase rune followed by lowercase; unannounced and untested

The new condition unicode.IsLower(prev) || unicode.IsDigit(prev) || (unicode.IsUpper(prev) && nextIsLower) only emits _ for those three cases. Every other previous rune now suppresses the underscore. That is a second behaviour change beyond the acronym handling the PR describes:

  • After a separator: foo_Bar was foo__bar, now foo_bar; _Foo was __foo, now _foo; foo-Bar was foo-_bar, now foo-bar. This is arguably an improvement, but it is not mentioned in the description or the doc comment, and no table entry covers it.
  • After a letter with no case, or a combining mark, the word boundary is lost entirely: 日本Go was 日本_go, now 日本go. A decomposed (NFD) cafe\u0301Bar now gives cafébar, while the precomposed (NFC) spelling of the same text still gives café_bar.

Evidence: _, -, U+65E5 (category Lo) and U+0301 (category Mn) satisfy none of the three terms, so the append at case.go:16 is skipped. The old body appended _ for every uppercase rune with i > 0. A Python port of both versions, run over every string up to length 6 over the alphabet a,B,C,1,_, showed the new output differs from the old only by dropped underscores, in two classes: previous rune uppercase and next not lowercase (the intended change), and previous rune _ (this finding). CamelToSnakeCase has no callers outside case_test.go, so there is no impact today.

Suggestion: decide which rule is intended and pin it with a test. At minimum add a foo_Bar entry. If a boundary after an uncased letter should be kept, test for a separator explicitly (for example !unicode.IsLetter(prev) && !unicode.IsMark(prev) && !unicode.IsDigit(prev)) instead of relying on the fall-through.

Agreed by 2 reviewer model(s).

out = append(out, '_')
}
}
out = append(out, unicode.ToLower(r))
}
Expand Down
16 changes: 16 additions & 0 deletions libs/textutil/case_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,22 @@ func TestCamelToSnakeCase(t *testing.T) {
input: "TestTestTest",
expected: "test_test_test",
},
{
input: "HTTPServer",
expected: "http_server",
},
{
input: "userID",
expected: "user_id",
},
{
input: "getHTTPResponseCode",
expected: "get_http_response_code",
},
{
input: "version2Beta",
expected: "version2_beta",
},

@Sankalp-Mittal Sankalp-Mittal Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nit: Other behavior changes (all-caps input, non-letter before a capital, digit after an acronym) are not in the test table

The new rule also changes outputs for inputs the table does not cover. ID -> id (was i_d) and ABC -> abc (was a_b_c) are the all-caps case. foo_Bar -> foo_bar (was foo__bar) is the case where a non-letter precedes a capital. These changes are plausible and arguably fixes, but the function is being redefined, so one or two table rows for them would lock them in. A HTTP2Server row would exercise the digit branch after an acronym. The brief also notes that the empty string and a single capital are untested.

Evidence: case_test.go:34-49 adds only HTTPServer, userID, getHTTPResponseCode and version2Beta. The foo_Bar result depends on the three-way condition at case.go:15 returning false for prev _, which was confirmed with a Python replay of the same rules.

Agreed by 2 reviewer model(s).

}

for _, c := range cases {
Expand Down
Loading