Repository navigation
[review-factory test] textutil: keep acronyms together in CamelToSnakeCase - #7633
Sankalp-Mittal wants to merge 1 commit into
Conversation
"HTTPServer" now becomes "http_server" instead of "h_t_t_p_server". A capital starts a new word only after a lowercase letter or digit, or when it begins a new capitalized word after a run of capitals. Co-authored-by: Isaac <no-reply@databricks.com>
✅ Integration test reportCommit: 037304b
Top 3 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
left a comment
Sankalp-Mittal
left a comment
There was a problem hiding this comment.
🤖 AI review from review-factory: 2 independent reviewer models (Claude Sonnet 5.5, Claude Fable 5.2) with cross-verification. Only findings that reviewers agreed on are posted.
Makes CamelToSnakeCase keep acronym runs together (HTTPServer becomes http_server); reviewers flagged an imprecise doc comment, an unannounced separator-handling change, and missing test rows.
Findings: 2 minor, 1 nit.
|
|
||
| import "unicode" | ||
|
|
||
| // CamelToSnakeCase converts camelCase or PascalCase to snake_case. A run of |
There was a problem hiding this comment.
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).
| 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) { |
There was a problem hiding this comment.
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_Barwasfoo__bar, nowfoo_bar;_Foowas__foo, now_foo;foo-Barwasfoo-_bar, nowfoo-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:
日本Gowas日本_go, now日本go. A decomposed (NFD)cafe\u0301Barnow givescafébar, while the precomposed (NFC) spelling of the same text still givescafé_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).
| { | ||
| input: "version2Beta", | ||
| expected: "version2_beta", | ||
| }, |
There was a problem hiding this comment.
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).
|
Closing: this was a test PR for an AI review tool. |
Changes
CamelToSnakeCasekeeps a run of capitals together as one word, soHTTPServerbecomeshttp_serveranduserIDbecomesuser_id, instead ofh_t_t_p_serveranduser_i_d.Why
This is a draft test PR for an automated review pipeline (review-factory). It is a small, self-contained change with no callers outside
libs/textutil, so it is safe to use as a test target. Please do not review or merge.Tests
Extended the table-driven
TestCamelToSnakeCasewith acronym, trailing-acronym, mid-word-acronym and digit cases;go test ./libs/textutil/passes.This pull request and its description were written by Isaac.