-
Notifications
You must be signed in to change notification settings - Fork 85
Fix batch split bug #257
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
base: main
Are you sure you want to change the base?
Fix batch split bug #257
Conversation
@microsoft-github-policy-service agree |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
==========================================
+ Coverage 74.88% 75.02% +0.13%
==========================================
Files 32 32
Lines 6471 6471
==========================================
+ Hits 4846 4855 +9
+ Misses 1335 1329 -6
+ Partials 290 287 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@shueybubbles could you take a look at this? |
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.
Pull Request Overview
This PR fixes a bug in the batch splitter where the word GOTO
was incorrectly recognized as a GO
batch delimiter by adding a forward lookup to ensure the separator isn’t part of a larger word, and adds a corresponding test case.
- Added a check in
hasPrefixFold
to reject matches where the next character is a letter. - Introduced a unit test to verify that
GOTO
isn’t split like aGO
batch command.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
batch/batch.go | Added a forward lookup in hasPrefixFold to ensure the next character isn’t a letter when matching. |
batch/batch_test.go | Added a test item verifying that GOTO doesn’t trigger a batch split at GO . |
Comments suppressed due to low confidence (1)
batch/batch_test.go:70
- [nitpick] Add a test case covering lowercase
go
(e.g.,go
vs.gotoflag
) to ensure the splitting logic is case-insensitive and handles lowercase separators correctly.
testItem{
if len(s) > len(sep) && unicode.IsLetter(rune(s[len(sep)])) { | ||
return false |
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.
When checking the character after the prefix, use utf8.DecodeRuneInString to properly handle multi-byte runes instead of casting a single byte to a rune.
if len(s) > len(sep) && unicode.IsLetter(rune(s[len(sep)])) { | |
return false | |
if len(s) > len(sep) { | |
r, _ := utf8.DecodeRuneInString(s[len(sep):]) | |
if unicode.IsLetter(r) { | |
return false | |
} |
Copilot uses AI. Check for mistakes.
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.
@heppu add some double byte char test cases to cover this.
Parsing breaks on
GOTO
word so I added ahead lookup with length validation to fix this. Without this fix running DB migration scripts isn't possible. PR waiting for this here.