Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
participant Client
participant EngineConfig
participant APIClient
participant APIServer as "API Server"
Client->>EngineConfig: quick_translate(content, api_key, engine_id, target_locale)
EngineConfig->>EngineConfig: validate engine_id\ntrim api_url\ngenerate sessionId
EngineConfig->>APIClient: init with api_url, engine_id, sessionId, api_key
APIClient->>APIServer: POST /process/localize\nHeaders: X-API-Key\nPayload: {engineId, sourceLocale, targetLocale, data, sessionId}
APIServer-->>APIClient: translated content
APIClient-->>Client: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
tests/test_engine.py (1)
40-43: Use a narrower expected exception intest_engine_id_required.Catching
Exceptionhere is too broad and can hide unrelated failures. Tightening the expected exception type will make this test more trustworthy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_engine.py` around lines 40 - 43, The test test_engine_id_required should assert a specific exception type instead of Exception; change the context manager from pytest.raises(Exception, match="engine_id") to a narrower exception that EngineConfig actually raises (e.g., pytest.raises(TypeError, match="engine_id") if EngineConfig.__init__ enforces a missing positional arg, or pytest.raises(ValueError, match="engine_id") if it validates and raises ValueError) so update the pytest.raises call in test_engine_id_required to the concrete exception type returned by EngineConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lingodotdev/engine.py`:
- Around line 205-212: The URL is built with raw self.config.engine_id which can
contain reserved characters; URL-encode the engine_id before composing url
(e.g., use urllib.parse.quote with safe='' on self.config.engine_id) and replace
direct insertion in the f-string with the encoded value, adding the necessary
import if missing; update the url assignment that currently uses
f"{self.config.api_url}/process/{self.config.engine_id}/localize" to use the
encoded engine_id variable instead.
- Around line 23-35: The validators validate_engine_id and validate_api_url call
string methods directly and can raise AttributeError for non-string inputs;
update them to first check the type and presence (e.g., ensure v is not None and
isinstance(v, str)) before using strip(), startswith(), or rstrip(), raise the
existing ValueError with the same messages for invalid or missing string values,
and return the cleaned string (strip()/rstrip("/")) when valid; reference the
validator methods validate_engine_id (for engine_id) and validate_api_url (for
api_url).
In `@tests/test_engine.py`:
- Around line 646-762: CI reports a Black formatting failure; run the Black
formatter over the repo (e.g., black .) and commit the updated formatting so the
tests in tests/test_engine.py (e.g., test_localize_chunk_url_and_body,
test_recognize_locale_url, test_full_localization_workflow) follow Black style;
then push the commit and re-run CI to confirm the Black check passes.
In `@tests/test_integration.py`:
- Around line 343-353: The test currently assumes mock_post.call_args_list
produced at least one call before indexing session_ids[0]; add an explicit
assertion before that access (e.g., assert len(session_ids) > 0 or assert
mock_post.called) with a clear message like "expected at least one POST call" so
failures are explicit, then keep the existing uniqueness and non-empty string
assertions; reference session_ids and mock_post.call_args_list when locating the
change.
---
Nitpick comments:
In `@tests/test_engine.py`:
- Around line 40-43: The test test_engine_id_required should assert a specific
exception type instead of Exception; change the context manager from
pytest.raises(Exception, match="engine_id") to a narrower exception that
EngineConfig actually raises (e.g., pytest.raises(TypeError, match="engine_id")
if EngineConfig.__init__ enforces a missing positional arg, or
pytest.raises(ValueError, match="engine_id") if it validates and raises
ValueError) so update the pytest.raises call in test_engine_id_required to the
concrete exception type returned by EngineConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e17f2f9-8874-4ffd-a922-86d752689270
📒 Files selected for processing (4)
README.mdsrc/lingodotdev/engine.pytests/test_engine.pytests/test_integration.py
Comment on lines
+343
to
353
| # Extract session IDs from all calls | ||
| session_ids = [] | ||
| for call in mock_post.call_args_list: | ||
| request_data = call[1]["json"] | ||
| workflow_id = request_data["params"]["workflowId"] | ||
| workflow_ids.append(workflow_id) | ||
| session_id = request_data["sessionId"] | ||
| session_ids.append(session_id) | ||
|
|
||
| # All workflow IDs should be the same | ||
| assert len(set(workflow_ids)) == 1 | ||
| assert len(workflow_ids[0]) > 0 # Should be a non-empty string | ||
| # All session IDs should be the same | ||
| assert len(set(session_ids)) == 1 | ||
| assert len(session_ids[0]) > 0 # Should be a non-empty string | ||
|
|
There was a problem hiding this comment.
Guard against empty call list before indexing session_ids[0].
At Line 352, indexing assumes at least one request was made. Add an explicit assertion first so regressions fail with a clear message.
Suggested test hardening
# All session IDs should be the same
+ assert session_ids, "Expected at least one localization request"
assert len(set(session_ids)) == 1
assert len(session_ids[0]) > 0 # Should be a non-empty string📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Extract session IDs from all calls | |
| session_ids = [] | |
| for call in mock_post.call_args_list: | |
| request_data = call[1]["json"] | |
| workflow_id = request_data["params"]["workflowId"] | |
| workflow_ids.append(workflow_id) | |
| session_id = request_data["sessionId"] | |
| session_ids.append(session_id) | |
| # All workflow IDs should be the same | |
| assert len(set(workflow_ids)) == 1 | |
| assert len(workflow_ids[0]) > 0 # Should be a non-empty string | |
| # All session IDs should be the same | |
| assert len(set(session_ids)) == 1 | |
| assert len(session_ids[0]) > 0 # Should be a non-empty string | |
| # Extract session IDs from all calls | |
| session_ids = [] | |
| for call in mock_post.call_args_list: | |
| request_data = call[1]["json"] | |
| session_id = request_data["sessionId"] | |
| session_ids.append(session_id) | |
| # All session IDs should be the same | |
| assert session_ids, "Expected at least one localization request" | |
| assert len(set(session_ids)) == 1 | |
| assert len(session_ids[0]) > 0 # Should be a non-empty string |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_integration.py` around lines 343 - 353, The test currently assumes
mock_post.call_args_list produced at least one call before indexing
session_ids[0]; add an explicit assertion before that access (e.g., assert
len(session_ids) > 0 or assert mock_post.called) with a clear message like
"expected at least one POST call" so failures are explicit, then keep the
existing uniqueness and non-empty string assertions; reference session_ids and
mock_post.call_args_list when locating the change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_integration.py (1)
349-351:⚠️ Potential issue | 🟡 MinorAdd an explicit non-empty assertion before
session_ids[0]access.This still assumes at least one mocked POST call. If no calls are made, the test fails with
IndexErrorinstead of a clear failure reason.💡 Suggested hardening
# All session IDs should be the same + assert session_ids, "Expected at least one localization request" assert len(set(session_ids)) == 1 assert len(session_ids[0]) > 0 # Should be a non-empty string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_integration.py` around lines 349 - 351, The test currently accesses session_ids[0] without verifying session_ids contains any entries; add an explicit non-empty check (e.g., assert len(session_ids) > 0 or assert session_ids) before the existing assertions so the failure is a clear assertion error instead of IndexError when no mocked POSTs occurred; update the assertions around session_ids and keep the subsequent checks (len(set(session_ids)) == 1 and len(session_ids[0]) > 0) unchanged.src/lingodotdev/engine.py (1)
25-33:⚠️ Potential issue | 🟡 MinorHarden
engine_idvalidator to handle non-string inputs gracefully.The current validator calls
v.strip()on line 30 without a type check. Withpre=True, non-string inputs (e.g.,EngineConfig(engine_id=123)) will reach the validator before Pydantic's type validation, causing anAttributeError: 'int' object has no attribute 'strip'instead of a clean validation error. Add a type check to convert unexpected input types to a clearValueError.🛠️ Suggested fix
`@validator`("engine_id", pre=True, always=True) `@classmethod` def validate_engine_id(cls, v: Optional[str]) -> Optional[str]: if v is None: return None + if not isinstance(v, str): + raise ValueError("engine_id must be a string") stripped = v.strip() if not stripped: return None return stripped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lingodotdev/engine.py` around lines 25 - 33, The validator validate_engine_id in the class (decorated with `@validator`("engine_id", pre=True, always=True)) currently calls v.strip() without checking v's type, which raises AttributeError for non-string inputs; update the validator to first check whether v is None, then if isinstance(v, str) use v.strip() and return None for empty strings, else raise a ValueError (or return None) with a clear message indicating engine_id must be a string so non-string inputs like integers produce a clean validation error rather than an AttributeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 225-226: The README entries for quick_translate and
quick_batch_translate list parameters in the wrong order; update the doc
signatures for the convenience methods quick_translate(...) and
quick_batch_translate(...) so their parameter order exactly matches the actual
function signatures in code (verify the real order in the implementation and
reorder the listed parameters—e.g., swap content/api_key or
target_locale/target_locales positions as necessary) to prevent misleading
positional calls.
---
Duplicate comments:
In `@src/lingodotdev/engine.py`:
- Around line 25-33: The validator validate_engine_id in the class (decorated
with `@validator`("engine_id", pre=True, always=True)) currently calls v.strip()
without checking v's type, which raises AttributeError for non-string inputs;
update the validator to first check whether v is None, then if isinstance(v,
str) use v.strip() and return None for empty strings, else raise a ValueError
(or return None) with a clear message indicating engine_id must be a string so
non-string inputs like integers produce a clean validation error rather than an
AttributeError.
In `@tests/test_integration.py`:
- Around line 349-351: The test currently accesses session_ids[0] without
verifying session_ids contains any entries; add an explicit non-empty check
(e.g., assert len(session_ids) > 0 or assert session_ids) before the existing
assertions so the failure is a clear assertion error instead of IndexError when
no mocked POSTs occurred; update the assertions around session_ids and keep the
subsequent checks (len(set(session_ids)) == 1 and len(session_ids[0]) > 0)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bac78be6-9b7b-4b39-afba-044ce78b81aa
📒 Files selected for processing (4)
README.mdsrc/lingodotdev/engine.pytests/test_engine.pytests/test_integration.py
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.
Description
Consolidate localization to a single
/process/localizeendpoint withengineIdpassed in the request body. Theengine_idis now an optional configuration option.Key changes:
engine_idis an optional field inEngineConfig,quick_translate, andquick_batch_translate(defaults toNone)/process/localize(instead of/process/{engine_id}/localize)engineIdis included in the request body only when providedengine_idvalues normalize toNoneapi_urlishttps://api.lingo.devX-API-KeyheadersourceLocale/targetLocalefields withsessionIdType of Change
Testing
Checklist
Summary by CodeRabbit
New Features
Documentation
Breaking Changes