fix: correct LastInsertId and Url casing in generated code#15
fix: correct LastInsertId and Url casing in generated code#15
Conversation
This fixes two bugs in FixAcronyms:
1. LastInsertId: The regex was incorrectly transforming res.LastInsertId() to
res.LastInsertID(). The Go standard library's sql.Result interface defines
LastInsertId() (lowercase d), so the generated MySQL wrappers failed to compile.
2. Url casing: With ExtraRules: true, gofumpt was uppercasing field names like
NewUrl to NewURL, but the underlying struct types generated by sqlc still
use NewUrl, causing type mismatches.
The fix disables ExtraRules in gofumpt and modifies the Id regex to exclude
'(' from the character class, preventing method calls like LastInsertId()
from being transformed.
|
This change is part of the following stack: Change managed by git-spice. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 32.28% 32.83% +0.54%
==========================================
Files 6 6
Lines 734 740 +6
==========================================
+ Hits 237 243 +6
Misses 486 486
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two specific issues within the code generation process: incorrect casing of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the acronym fixer was incorrectly transforming method calls like LastInsertId() to LastInsertID(). The fix involves modifying the regex logic in generator/helpers.go to exclude the opening parenthesis when matching the "Id" acronym, preventing method calls from being altered. New test cases have been added in generator/helpers_test.go to validate this behavior. Additionally, the ExtraRules for gofumpt formatting were disabled. A review comment suggests a performance improvement for the FixAcronyms function by pre-compiling regular expressions outside the loop.
| // For Id, exclude '(' to preserve method calls like res.LastInsertId() | ||
| nonLetterClass := `[^A-Za-z]` | ||
| if a.pattern == "Id" { | ||
| nonLetterClass = `[^A-Za-z(]` | ||
| } | ||
|
|
||
| regexNonLetter := regexp.MustCompile(`([a-z])(` + a.pattern + `)(` + nonLetterClass + `)`) |
There was a problem hiding this comment.
While this logic correctly fixes the issue, compiling regular expressions inside this for loop is inefficient. The compilation happens on every call to FixAcronyms for every acronym, which can be a performance bottleneck.
To improve this, consider pre-compiling all the regular expressions once at package initialization using an init() function. You could store them in a global variable (e.g., a slice of structs, where each struct holds the compiled regexes for an acronym). This ensures the expensive compilation step is performed only once when the package is loaded.
|
this is just too messy and error prone, I decided to drop this feature... |
2 participants
This fixes two bugs in FixAcronyms:
LastInsertId: The regex was incorrectly transforming res.LastInsertId() to
res.LastInsertID(). The Go standard library's sql.Result interface defines
LastInsertId() (lowercase d), so the generated MySQL wrappers failed to compile.
Url casing: With ExtraRules: true, gofumpt was uppercasing field names like
NewUrl to NewURL, but the underlying struct types generated by sqlc still
use NewUrl, causing type mismatches.
The fix disables ExtraRules in gofumpt and modifies the Id regex to exclude
'(' from the character class, preventing method calls like LastInsertId()
from being transformed.