Initial Core Implementation of Lingo.dev Go SDK#3
Open
iYashMaurya wants to merge 30 commits intolingodotdev:mainfrom
Open
Initial Core Implementation of Lingo.dev Go SDK#3iYashMaurya wants to merge 30 commits intolingodotdev:mainfrom
iYashMaurya wants to merge 30 commits intolingodotdev:mainfrom
Conversation
…shift API validation to client layer
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a complete Go SDK for Lingo.dev including client, config, models, errors, chunking and retry logic, high-level localization APIs, extensive unit/integration tests, CI/release workflows, changelog tooling, README/docs, examples, and Apache-2.0 license. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant SDK as Lingo SDK
participant HTTP as HTTP Client
participant API as Lingo.dev API
App->>SDK: LocalizeText(ctx, text, params)
SDK->>SDK: CountWords & split into chunks
loop per chunk
SDK->>HTTP: POST /i18n (ctx, auth, chunk)
HTTP->>API: Send request
alt 5xx response
API-->>HTTP: 5xx
HTTP->>HTTP: wait rgba(255,165,0,0.5) (exponential backoff)
HTTP->>API: retry (up to 3)
end
API-->>HTTP: 200 + JSON
HTTP-->>SDK: parsed response
end
SDK->>SDK: merge chunk responses
SDK-->>App: localized result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
go.mod (1)
6-7: Remove// indirectfrom directly imported dependencies.Lines 6-7 are directly used in runtime code (
engine.go), so they should be declared as direct requirements to keep module metadata accurate.Suggested diff
require ( - github.com/matoous/go-nanoid/v2 v2.1.0 // indirect - golang.org/x/sync v0.20.0 // indirect + github.com/matoous/go-nanoid/v2 v2.1.0 + golang.org/x/sync v0.20.0 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 6 - 7, go.mod currently marks github.com/matoous/go-nanoid/v2 and golang.org/x/sync as indirect, but they are imported/used directly (see engine.go); remove the trailing "// indirect" markers for the entries for github.com/matoous/go-nanoid/v2 v2.1.0 and golang.org/x/sync v0.20.0 so they become direct requirements, then run "go mod tidy" (or equivalent) to normalize the module file and verify no other entries revert to indirect.model.go (1)
5-17: Consider aliasing the nestedReferencemap type for readability.The same complex type appears in multiple places (e.g., Line 9 and Line 16). A small type alias would make future changes safer and clearer.
Suggested diff
+type ReferenceMap map[string]map[string]any + type LocalizationParams struct { SourceLocale *string `json:"source_locale,omitempty"` TargetLocale string `json:"target_locale"` Fast *bool `json:"fast,omitempty"` - Reference map[string]map[string]any `json:"reference,omitempty"` + Reference ReferenceMap `json:"reference,omitempty"` } type requestData struct { Param parameter `json:"params"` Locale locale `json:"locale"` Data any `json:"data"` - Reference map[string]map[string]any `json:"reference,omitempty"` + Reference ReferenceMap `json:"reference,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model.go` around lines 5 - 17, Introduce a type alias for the nested map (e.g., type NestedReference = map[string]map[string]any) and replace the inline map type usages in the structs: update LocalizationParams.Reference and requestData.Reference to use the new NestedReference alias. Ensure the alias is declared at package level above the struct definitions and update any other occurrences of map[string]map[string]any in this file to use the alias for consistency and easier future changes..github/workflows/pr.yml (2)
50-56: Codecov upload may silently fail without a token.For public repositories,
codecov-action@v4can work without a token, but for private repositories or to ensure reliable uploads, consider addingCODECOV_TOKENfrom secrets. Thefail_ci_if_error: falsesetting handles failures gracefully, which is appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr.yml around lines 50 - 56, The Codecov upload step using codecov/codecov-action@v4 may fail silently without a token for private repos; update the GitHub Actions step that uses codecov/codecov-action@v4 to include the token input (e.g., set token: ${{ secrets.CODECOV_TOKEN }}) in the with: block while keeping fail_ci_if_error: false, so the action will use CODECOV_TOKEN from secrets when present and still not fail the CI on upload errors.
12-13: Consider pinning Go versions to specific patch releases for reproducible CI.Using
'1.21'and'1.22'will resolve to the latest patch versions, which is generally fine but may cause unexpected behavior if a patch release introduces breaking changes. For stricter reproducibility, consider using explicit versions like'1.21.x'or'1.22.x'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr.yml around lines 12 - 13, The CI matrix currently lists abstract Go versions under the `matrix` key with `go-version: ['1.21', '1.22']`; update this to pin patch-level versions (e.g., replace those entries with explicit patch pins like `1.21.x` or concrete patch releases such as `1.21.16`, `1.22.7`) so the `go-version` matrix entries resolve deterministically; modify the `go-version` array in the workflow to the chosen pinned values to ensure reproducible CI runs..github/workflows/release.yml (2)
48-52: Redundant condition on integration job.The condition
github.event_name == 'push' && github.ref == 'refs/heads/main'is redundant since the workflow only triggers onpushtomain(lines 3-5). This can be simplified or removed entirely.Proposed simplification
integration: name: Integration Tests runs-on: ubuntu-latest needs: test - if: github.event_name == 'push' && github.ref == 'refs/heads/main'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 48 - 52, The integration job's conditional check on github.event_name and github.ref is redundant; in the workflow remove the unnecessary if: github.event_name == 'push' && github.ref == 'refs/heads/main' from the integration job (named "integration") so the job relies on the workflow-level trigger, or if you intended a branch guard only keep the github.ref check; update the job block for "integration" to drop or simplify the if line accordingly.
114-137: Version parsing may fail for non-standard tags.The version parsing at lines 119-120 assumes tags follow strict
vMAJOR.MINOR.PATCHformat. IfLATEST_TAGisv0.0.0(the fallback) or has a different format (e.g.,v1.0.0-beta), theIFS='.' readmay produce unexpected results.Consider adding validation or using a more robust parsing approach for production use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 114 - 137, The current tag parsing (variables LATEST_TAG -> VERSION -> IFS read into MAJOR/MINOR/PATCH) can break for non-SEMVER or pre-release tags (e.g., v1.0.0-beta); update the logic that sets VERSION/MAJOR/MINOR/PATCH to validate and normalize the tag first: strip only a leading "v" from LATEST_TAG, then match VERSION against a semver regex to capture numeric MAJOR, MINOR, PATCH and fall back to "0.0.0" when no numeric semver is found; ensure the downstream NEXT_VERSION construction uses those validated numeric captures so PRE-RELEASE or malformed tags don't cause incorrect splitting. Reference symbols: LATEST_TAG, VERSION, MAJOR, MINOR, PATCH, NEXT_VERSION, and steps.check_tag.outputs.latest_tag.config.go (1)
20-28: URL validation only checks prefix, not full URL validity.The
SetURLfunction validates that the URL starts withhttp://orhttps://but doesn't verify that the rest of the URL is well-formed. While this is a reasonable minimum check, malformed URLs likehttps://(empty host) orhttps://[invalidwould pass validation but fail at runtime.Consider using
url.Parsefor stricter validation if invalid URLs cause confusing errors downstream.Optional: Stricter URL validation
+import ( + "net/url" + "strings" +) func SetURL(url string) ConfigOption { return func(c *Config) error { if !(strings.HasPrefix(url, "http://") || strings.HasPrefix(url, "https://")) { return &ValueError{Message: "lingo: api url must be a valid http/https url"} } + parsed, err := url.Parse(url) + if err != nil || parsed.Host == "" { + return &ValueError{Message: "lingo: api url must be a valid http/https url"} + } c.APIURL = url return nil } }Note: The variable name
urlshadows thenet/urlpackage, so you'd need to rename it (e.g.,apiURL).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.go` around lines 20 - 28, The SetURL ConfigOption currently only checks the prefix; update the validator in SetURL (returned function) to perform proper parsing using net/url (rename the parameter from url to apiURL to avoid shadowing) and call url.Parse or url.ParseRequestURI, then verify the parsed result has a non-empty Scheme and Host (and no parse error); if parsing fails or Host is empty return the ValueError with the same message, otherwise assign c.APIURL = apiURL and return nil.engine.go (1)
35-38: Cap concurrent outbound calls.Each of these
errgrouppaths can launch one HTTP request per chunk, locale, or object. Large inputs will fan out unbounded work against both the engine and the caller's transport;errgroup.Group.SetLimitis available here and would let you keep concurrency configurable.Also applies to: 259-260, 292-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine.go` around lines 35 - 38, The errgroup paths that create g via errgroup.WithContext launch one HTTP request per chunk/locale/object and currently run unbounded; update each errgroup usage (the g returned from errgroup.WithContext in the concurrent branch and the other errgroup blocks referenced) to call g.SetLimit(n) with a configurable concurrency limit (e.g., use an existing config value or add a new int parameter/constant like maxConcurrentEngineCalls) before spawning goroutines so outbound calls are capped; ensure the limit is applied to every errgroup instance (the g variable created in those blocks) and wire the config through the function signature or package-level setting as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 144-151: The "Commit changelog" job currently performs a direct
git push using the default GITHUB_TOKEN which cannot bypass branch protection,
so update the step to either (A) use a PAT/bot token stored in a secret (e.g.,
replace git push with setting git remote to https://x-access-token:${{
secrets.PERSONAL_ACCESS_TOKEN }}@github.com/${{ github.repository }}.git before
pushing) and ensure the secret has repo:status/contents/write and bypass
protections, or (B) document in the workflow README that branch protection must
allow github-actions[bot] pushes; modify the step named "Commit changelog"
(which checks steps.check_tag.outputs.release_needed and uses
steps.next_version.outputs.version) to use the chosen approach so the git push
will succeed under protected branches.
In `@client.go`:
- Around line 138-156: CountWords currently only handles []any and
map[string]any and misses concrete Go collections like []string and
map[string]string; update CountWords (function name: CountWords) to first keep
the existing fast type switches (string, []any, map[string]any) and then add a
reflect-based fallback: use reflect.ValueOf(payload) to detect Kind() == Slice
|| Array and iterate elements calling CountWords on each, and detect Kind() ==
Map and iterate map values calling CountWords on each (skip nils); preserve the
string branch using strings.Fields and return 0 for unsupported/nil inputs so
nested concrete slices/maps (e.g., []string, map[string]string) contribute
correctly to the total.
- Around line 64-84: The retry loop currently uses time.Sleep() (in the block
around attempt/maxRetries) which ignores ctx cancellation; change the sleep to a
context-aware wait (use select { case <-ctx.Done(): return nil,
&RuntimeError{Message: fmt.Sprintf("lingo: request aborted: %v", ctx.Err())}
case <-time.After(time.Duration(1<<uint(attempt-1)) * time.Second): } ) and also
check ctx.Done() before creating the next request (i.e., before
http.NewRequestWithContext and before calling c.httpClient.Do) so that when ctx
is cancelled or its deadline expires the function returns promptly with the
context error instead of continuing retries; update uses of time.Sleep, attempt,
maxRetries, c.httpClient.Do, and lastErr accordingly.
In `@engine.go`:
- Around line 28-31: The code currently collapses an optional pointer field
LocalizationParams.Fast (params.Fast) into a plain bool (fast) losing the
nil-vs-false distinction; update the API and call chain so the optionality is
preserved: change parameter.Fast to *bool (and add `omitempty` where structs are
serialized), update the localizeChunk signature and any callers to accept and
forward a *bool instead of converting to bool, and remove the intermediate
conversion (the `fast := false` / `if params.Fast != nil { fast = *params.Fast
}` block) so nil remains nil and explicit false remains false.
In `@go.mod`:
- Line 3: Update the go.mod module directive from `go 1.25.0` to `go 1.21` so
the module's declared minimum Go version matches CI/test targets; edit the `go`
directive line in go.mod (replace the existing `go 1.25.0` entry) to read `go
1.21`.
In `@Makefile`:
- Line 1: The .PHONY declaration lists a non-existent target "test-unit"; remove
"test-unit" from the .PHONY line (the symbol to change is the .PHONY
declaration) or alternatively add a matching "test-unit" target to the Makefile;
update the .PHONY entry accordingly so every listed phony target (e.g., test,
test-integration, test-coverage, build, vet, fmt, lint, clean, help) corresponds
to an actual target.
In `@README.md`:
- Line 18: Replace the garbled replacement character on the markdown line
containing "**Functional options** pattern for clean, extensible configuration"
with the intended emoji (e.g., the gear/config emoji ⚙️); locate the text
matching that exact line and substitute the � character with ⚙️ so the README
displays the correct emoji.
In `@tests/integration_test.go`:
- Around line 255-289: Remove the flaky wall-clock assertion from the
TestRealAPI_ConcurrentVsSequential test: keep the setup and both calls to
client.LocalizeObject to verify they succeed (no errors) but delete the
timing-based comparisons that use seqDuration, concDuration, and threshold and
the final t.Errorf; ensure the test only asserts functional correctness (no
errors returned) for sequential and concurrent runs in the
TestRealAPI_ConcurrentVsSequential function.
---
Nitpick comments:
In @.github/workflows/pr.yml:
- Around line 50-56: The Codecov upload step using codecov/codecov-action@v4 may
fail silently without a token for private repos; update the GitHub Actions step
that uses codecov/codecov-action@v4 to include the token input (e.g., set token:
${{ secrets.CODECOV_TOKEN }}) in the with: block while keeping fail_ci_if_error:
false, so the action will use CODECOV_TOKEN from secrets when present and still
not fail the CI on upload errors.
- Around line 12-13: The CI matrix currently lists abstract Go versions under
the `matrix` key with `go-version: ['1.21', '1.22']`; update this to pin
patch-level versions (e.g., replace those entries with explicit patch pins like
`1.21.x` or concrete patch releases such as `1.21.16`, `1.22.7`) so the
`go-version` matrix entries resolve deterministically; modify the `go-version`
array in the workflow to the chosen pinned values to ensure reproducible CI
runs.
In @.github/workflows/release.yml:
- Around line 48-52: The integration job's conditional check on
github.event_name and github.ref is redundant; in the workflow remove the
unnecessary if: github.event_name == 'push' && github.ref == 'refs/heads/main'
from the integration job (named "integration") so the job relies on the
workflow-level trigger, or if you intended a branch guard only keep the
github.ref check; update the job block for "integration" to drop or simplify the
if line accordingly.
- Around line 114-137: The current tag parsing (variables LATEST_TAG -> VERSION
-> IFS read into MAJOR/MINOR/PATCH) can break for non-SEMVER or pre-release tags
(e.g., v1.0.0-beta); update the logic that sets VERSION/MAJOR/MINOR/PATCH to
validate and normalize the tag first: strip only a leading "v" from LATEST_TAG,
then match VERSION against a semver regex to capture numeric MAJOR, MINOR, PATCH
and fall back to "0.0.0" when no numeric semver is found; ensure the downstream
NEXT_VERSION construction uses those validated numeric captures so PRE-RELEASE
or malformed tags don't cause incorrect splitting. Reference symbols:
LATEST_TAG, VERSION, MAJOR, MINOR, PATCH, NEXT_VERSION, and
steps.check_tag.outputs.latest_tag.
In `@config.go`:
- Around line 20-28: The SetURL ConfigOption currently only checks the prefix;
update the validator in SetURL (returned function) to perform proper parsing
using net/url (rename the parameter from url to apiURL to avoid shadowing) and
call url.Parse or url.ParseRequestURI, then verify the parsed result has a
non-empty Scheme and Host (and no parse error); if parsing fails or Host is
empty return the ValueError with the same message, otherwise assign c.APIURL =
apiURL and return nil.
In `@engine.go`:
- Around line 35-38: The errgroup paths that create g via errgroup.WithContext
launch one HTTP request per chunk/locale/object and currently run unbounded;
update each errgroup usage (the g returned from errgroup.WithContext in the
concurrent branch and the other errgroup blocks referenced) to call
g.SetLimit(n) with a configurable concurrency limit (e.g., use an existing
config value or add a new int parameter/constant like maxConcurrentEngineCalls)
before spawning goroutines so outbound calls are capped; ensure the limit is
applied to every errgroup instance (the g variable created in those blocks) and
wire the config through the function signature or package-level setting as
appropriate.
In `@go.mod`:
- Around line 6-7: go.mod currently marks github.com/matoous/go-nanoid/v2 and
golang.org/x/sync as indirect, but they are imported/used directly (see
engine.go); remove the trailing "// indirect" markers for the entries for
github.com/matoous/go-nanoid/v2 v2.1.0 and golang.org/x/sync v0.20.0 so they
become direct requirements, then run "go mod tidy" (or equivalent) to normalize
the module file and verify no other entries revert to indirect.
In `@model.go`:
- Around line 5-17: Introduce a type alias for the nested map (e.g., type
NestedReference = map[string]map[string]any) and replace the inline map type
usages in the structs: update LocalizationParams.Reference and
requestData.Reference to use the new NestedReference alias. Ensure the alias is
declared at package level above the struct definitions and update any other
occurrences of map[string]map[string]any in this file to use the alias for
consistency and easier future changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f554ddca-0ff8-4774-ae8f-cbfcad7bf021
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.chglog/CHANGELOG.tpl.md.chglog/config.yml.github/pull_request_template.md.github/workflows/pr.yml.github/workflows/release.yml.gitignoreCHANGELOG.mdLICENSEMakefileREADME.mdclient.goconfig.goengine.goerror.goexamples/main.gogo.modmodel.gotests/client_test.gotests/integration_test.go
Comment on lines
+144
to
+151
| - name: Commit changelog | ||
| if: steps.check_tag.outputs.release_needed == 'true' | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git add CHANGELOG.md | ||
| git commit -m "chore: update CHANGELOG for ${{ steps.next_version.outputs.version }}" || echo "No changes to commit" | ||
| git push |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about branch protection or release workflow setup
fd -e md | xargs -I {} grep -l -i "branch protection\|release workflow\|bot\|token" {} 2>/dev/null || echo "No relevant documentation found"Repository: lingodotdev/sdk-go
Length of output: 93
🏁 Script executed:
cat -n .github/workflows/release.ymlRepository: lingodotdev/sdk-go
Length of output: 6021
🏁 Script executed:
fd -type f -name "*.yml" -o -name "*.yaml" | xargs grep -l "branch.*protection\|protection.*rule\|bypass" 2>/dev/null || echo "No branch protection config found"Repository: lingodotdev/sdk-go
Length of output: 283
🏁 Script executed:
find . -type f \( -name "*.json" -o -name "*.toml" -o -name "*.yaml" -o -name "*.yml" \) | head -20Repository: lingodotdev/sdk-go
Length of output: 141
🏁 Script executed:
# Check git configuration or any special setup
find .github -type f | head -20Repository: lingodotdev/sdk-go
Length of output: 149
🏁 Script executed:
ls -la | grep -i readmeRepository: lingodotdev/sdk-go
Length of output: 121
🏁 Script executed:
git log --oneline -20 | head -20Repository: lingodotdev/sdk-go
Length of output: 123
🏁 Script executed:
head -100 README.mdRepository: lingodotdev/sdk-go
Length of output: 2889
Direct push requires either a PAT with bypass permissions or disabling branch protection for the bot.
The workflow uses GITHUB_TOKEN with contents: write, which does not bypass branch protection rules requiring pull requests or reviews. If the main branch is protected with PR requirements, the git push at line 151 will fail. Either:
- Use a Personal Access Token (PAT) with appropriate scopes, or
- Create a bot account with bypass permissions and use its token, or
- Configure branch protection rules to allow the
github-actions[bot]account to push directly, or - Document this as a prerequisite for the release workflow
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 144 - 151, The "Commit changelog"
job currently performs a direct git push using the default GITHUB_TOKEN which
cannot bypass branch protection, so update the step to either (A) use a PAT/bot
token stored in a secret (e.g., replace git push with setting git remote to
https://x-access-token:${{ secrets.PERSONAL_ACCESS_TOKEN }}@github.com/${{
github.repository }}.git before pushing) and ensure the secret has
repo:status/contents/write and bypass protections, or (B) document in the
workflow README that branch protection must allow github-actions[bot] pushes;
modify the step named "Commit changelog" (which checks
steps.check_tag.outputs.release_needed and uses
steps.next_version.outputs.version) to use the chosen approach so the git push
will succeed under protected branches.
Comment on lines
+28
to
+31
| fast := false | ||
| if params.Fast != nil { | ||
| fast = *params.Fast | ||
| } |
There was a problem hiding this comment.
Don't turn Fast=nil into fast:false.
LocalizationParams.Fast is optional, but this conversion drops the distinction between omitted and explicit false. Because parameter.Fast is a plain bool, callers can never defer to the API default once this code runs.
💡 Suggested direction
- fast := false
- if params.Fast != nil {
- fast = *params.Fast
- }
+ fast := params.FastlocalizeChunk and parameter.Fast need the same pointer-preserving change outside this hunk (*bool + omitempty).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine.go` around lines 28 - 31, The code currently collapses an optional
pointer field LocalizationParams.Fast (params.Fast) into a plain bool (fast)
losing the nil-vs-false distinction; update the API and call chain so the
optionality is preserved: change parameter.Fast to *bool (and add `omitempty`
where structs are serialized), update the localizeChunk signature and any
callers to accept and forward a *bool instead of converting to bool, and remove
the intermediate conversion (the `fast := false` / `if params.Fast != nil { fast
= *params.Fast }` block) so nil remains nil and explicit false remains false.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Makefile (2)
20-23: Prefer skip semantics over hard-fail when API key is missing.PR objectives call out integration tests being skipped when
LINGODOTDEV_API_KEYis unset; exiting with code 1 can fail workflows that invoke this target unconditionally.Suggested patch
test-integration: `@if` [ -z "$$LINGODOTDEV_API_KEY" ]; then \ - echo "Error: LINGODOTDEV_API_KEY is not set"; \ - exit 1; \ + echo "Skipping integration tests: LINGODOTDEV_API_KEY is not set"; \ + exit 0; \ fi go test ./tests/... -run "TestRealAPI" -v -timeout 120s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 20 - 23, The Makefile currently hard-fails when LINGODOTDEV_API_KEY is unset (the conditional checking $$LINGODOTDEV_API_KEY) which breaks workflows; change the behavior to skip instead: when the environment variable is empty, print a clear "skipping integration tests" message and return success (use exit 0 or the shell built-in true) instead of exit 1 so callers that invoke this target unconditionally won't fail. Ensure you update the conditional block that references $$LINGODOTDEV_API_KEY accordingly.
1-1: Consider adding a conventionalalltarget.This would satisfy the
minphonystatic warning and provide a standard entrypoint (make all) for contributors.Suggested patch
-.PHONY: test test-integration test-coverage test-coverage-html build vet fmt lint clean help +.PHONY: all test test-integration test-coverage test-coverage-html build vet fmt lint clean help + +all: lint test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, The Makefile is missing a conventional default "all" target which triggers the minphony warning; add an explicit "all" target and include it in the .PHONY list. Modify the .PHONY declaration (.PHONY: test test-integration test-coverage test-coverage-html build vet fmt lint clean help) to include "all", and add a new target like "all: build" (or another appropriate aggregate target) so that running "make" or "make all" invokes the expected default build step.client.go (3)
44-48: Byte-based truncation may produce invalid UTF-8 in error messages.
len(text)counts bytes, not runes. For multi-byte UTF-8 characters (e.g., emoji, CJK), slicing at byte position 200 can split a character mid-sequence, producing malformed output in error messages.🛠️ Suggested fix using rune-aware truncation
func TruncateResponse(text string) string { - if len(text) > MaxResponseLength { - return text[:MaxResponseLength] + "..." + runes := []rune(text) + if len(runes) > MaxResponseLength { + return string(runes[:MaxResponseLength]) + "..." } return text }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 44 - 48, TruncateResponse currently uses byte-based len(text) and byte-slicing which can split multi-byte UTF-8 characters; change it to operate on runes: convert text to a []rune, check len(runes) against MaxResponseLength, and return string(runes[:MaxResponseLength]) + "..." when truncating so output remains valid UTF-8; keep the TruncateResponse signature and MaxResponseLength semantics (counting runes) consistent with other code.
34-39: Consider allowing customhttp.Clientinjection for testability.The client creates its own
http.Clientwith a hardcoded 60s timeout. For advanced use cases (custom transport, proxy configuration, or testing with mock servers), allowing an optionalhttp.Clientinjection via aConfigOptionwould improve flexibility.This could be added as a future enhancement:
// SetHTTPClient configures a custom HTTP client. func SetHTTPClient(client *http.Client) ConfigOption { return func(c *Config) error { // Store in Config, then use in NewClient return nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 34 - 39, The constructor currently hardcodes an http.Client inside Client (see Client struct field httpClient and the object creation block) which reduces testability; add a ConfigOption type and a SetHTTPClient(client *http.Client) ConfigOption that stores the provided client on Config, then update NewClient (or whichever function creates the Client) to use cfg.HTTPClient if non-nil and fall back to the existing &http.Client{Timeout: 60*time.Second} otherwise; ensure the Config and Client types include the new field names (e.g., Config.HTTPClient and Client.httpClient) so callers can inject custom transports, proxies, or mock clients.
189-199: Consider incremental word counting for efficiency.
CountWords(currentChunk)rescans the entire chunk on every iteration, resulting in O(n²) word counting across all keys. Given the batch size caps (max 250 items, 2500 words), performance impact is likely minimal in practice.♻️ Optional: Track running word count incrementally
func (c *Client) ExtractChunks(payload map[string]any) []map[string]any { total := len(payload) processed := 0 var result []map[string]any currentChunk := make(map[string]any) var currentItemCount int + var currentWordCount int for key, value := range payload { currentChunk[key] = value currentItemCount++ - currentChunkSize := CountWords(currentChunk) + currentWordCount += CountWords(value) processed++ - if currentChunkSize > c.config.IdealBatchItemSize || currentItemCount >= c.config.BatchSize || processed == total { + if currentWordCount > c.config.IdealBatchItemSize || currentItemCount >= c.config.BatchSize || processed == total { result = append(result, currentChunk) currentChunk = make(map[string]any) currentItemCount = 0 + currentWordCount = 0 } } return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 189 - 199, The loop currently calls CountWords(currentChunk) on every iteration causing O(n²) scanning; replace that with an incremental running word count: maintain a variable (e.g., currentChunkWordCount) updated when you add a key/value by adding CountWordsOfValue(value) (implement a helper to count words for the single value or use existing CountWords on the value only), use currentChunkWordCount in the condition instead of CountWords(currentChunk), and reset currentChunkWordCount to 0 whenever you start a new currentChunk (same spots where currentChunk is reinitialized). Update any references to currentChunkSize to use currentChunkWordCount and ensure processed/total logic remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client.go`:
- Around line 119-123: The RuntimeError path returns an error message that omits
reasonPhrase causing inconsistent formatting versus the ValueError path; update
the else branch that constructs &RuntimeError{Message: ...} to include
reasonPhrase alongside responsePreview (similar to the ValueError case) so all
non-5xx error messages include both reasonPhrase and responsePreview; locate the
construction of &RuntimeError in the function handling resp (variables resp,
reasonPhrase, responsePreview) and change the fmt.Sprintf format string to
include reasonPhrase and preserve StatusCode: resp.StatusCode.
---
Nitpick comments:
In `@client.go`:
- Around line 44-48: TruncateResponse currently uses byte-based len(text) and
byte-slicing which can split multi-byte UTF-8 characters; change it to operate
on runes: convert text to a []rune, check len(runes) against MaxResponseLength,
and return string(runes[:MaxResponseLength]) + "..." when truncating so output
remains valid UTF-8; keep the TruncateResponse signature and MaxResponseLength
semantics (counting runes) consistent with other code.
- Around line 34-39: The constructor currently hardcodes an http.Client inside
Client (see Client struct field httpClient and the object creation block) which
reduces testability; add a ConfigOption type and a SetHTTPClient(client
*http.Client) ConfigOption that stores the provided client on Config, then
update NewClient (or whichever function creates the Client) to use
cfg.HTTPClient if non-nil and fall back to the existing &http.Client{Timeout:
60*time.Second} otherwise; ensure the Config and Client types include the new
field names (e.g., Config.HTTPClient and Client.httpClient) so callers can
inject custom transports, proxies, or mock clients.
- Around line 189-199: The loop currently calls CountWords(currentChunk) on
every iteration causing O(n²) scanning; replace that with an incremental running
word count: maintain a variable (e.g., currentChunkWordCount) updated when you
add a key/value by adding CountWordsOfValue(value) (implement a helper to count
words for the single value or use existing CountWords on the value only), use
currentChunkWordCount in the condition instead of CountWords(currentChunk), and
reset currentChunkWordCount to 0 whenever you start a new currentChunk (same
spots where currentChunk is reinitialized). Update any references to
currentChunkSize to use currentChunkWordCount and ensure processed/total logic
remains unchanged.
In `@Makefile`:
- Around line 20-23: The Makefile currently hard-fails when LINGODOTDEV_API_KEY
is unset (the conditional checking $$LINGODOTDEV_API_KEY) which breaks
workflows; change the behavior to skip instead: when the environment variable is
empty, print a clear "skipping integration tests" message and return success
(use exit 0 or the shell built-in true) instead of exit 1 so callers that invoke
this target unconditionally won't fail. Ensure you update the conditional block
that references $$LINGODOTDEV_API_KEY accordingly.
- Line 1: The Makefile is missing a conventional default "all" target which
triggers the minphony warning; add an explicit "all" target and include it in
the .PHONY list. Modify the .PHONY declaration (.PHONY: test test-integration
test-coverage test-coverage-html build vet fmt lint clean help) to include
"all", and add a new target like "all: build" (or another appropriate aggregate
target) so that running "make" or "make all" invokes the expected default build
step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26f70cce-04fb-49d4-9119-cc9078ecb268
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
Makefileclient.goerror.gogo.modtests/integration_test.go
✅ Files skipped from review due to trivial changes (2)
- error.go
- tests/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR delivers the complete, production-ready Lingo.dev Go SDK. It achieves full feature parity with the existing Python, JavaScript, and PHP SDKs (excluding HTML processing, which remains JS-only).
The SDK has been built from the ground up to follow Go best practices, featuring robust concurrency, context propagation, comprehensive error handling, and fully automated CI/CD pipelines.
Closes lingodotdev/lingo.dev#1073
🚀 What's Included
Core SDK Features
LocalizeText,LocalizeObject,LocalizeChat,BatchLocalizeText,BatchLocalizeObjects,RecognizeLocale, andWhoAmI.context.Contextas the first parameter, propagating all the way down tohttp.NewRequestWithContextfor reliable request cancellation.golang.org/x/sync/errgroup.ValueErrorfor validation/400 errors andRuntimeError(with aStatusCodefield) for server/network errors.SetURL,SetBatchSize,SetIdealBatchItemSize) for clean client initialization.🧪 Testing (82.6% Coverage)
httptest.NewServercovering config validation, chunking, truncation, error types, and core endpoints.LINGODOTDEV_API_KEYis not set).⚙️ CI/CD & Automation
vet,gofmtchecks, conditional integration tests, and coverage uploads to Codecov.git-chglog, and GitHub release creation.📚 Documentation
examples/main.goand aMakefilewith targets fortest,test-integration,test-coverage,build,vet,fmt,lint, andclean.🏗️ Architecture & Request Flow
📝 Technical Notes for Reviewers
do()method returns amap[string]anyrepresenting the full JSON response. This allows divergent response structures (like/recognizevs/i18n) to pass through the same robust HTTP and retry layer. Callers extract exactly what they need.errgroup.WithContextto manage parallel execution, ensuring automatic cancellation of sibling routines if one fails.📦 Dependencies
golang.org/x/sync— Utilizederrgroupfor safe concurrent chunk and batch processing.github.com/matoous/go-nanoid/v2— Used for robust workflow ID generation.Summary by CodeRabbit
New Features
Documentation
Tests
Chores