#37 : Add some tests for Asciidoc and Quickbook format.#38
Merged
wpak-ai merged 17 commits intocppalliance:developfrom May 8, 2026
Merged
#37 : Add some tests for Asciidoc and Quickbook format.#38wpak-ai merged 17 commits intocppalliance:developfrom
wpak-ai merged 17 commits intocppalliance:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds QuickBook parser unit tests and fixtures, QuickBookFormat integration tests, and updates QuickBook and AsciiDoc merge logic to normalize (source, context) matching and apply DB targets with fuzzy handling. ChangesQuickBook Format Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@weblate/utils/tests/test_quickbook.py`:
- Around line 7-95: Replace the unittest-based classes QuickBookParserTest and
QuickBookConversionTest with module-level pytest test functions: remove "import
unittest", change each method (e.g., test_find_bracket_end_nested,
test_parse_bracket_keyword_section_with_id, test_skip_include_and_parse_heading,
test_paragraph_soft_wrap_joined, test_indented_code_block_skipped,
test_section_title_and_body, test_inline_bracket_on_wrapped_line,
test_qbk_to_po_locations, test_po_to_qbk_applies_translation,
test_po_to_qbk_fallback_untranslated) into standalone def functions, use plain
assert expressions instead of self.assertEqual/self.assertIn/self.assertTrue,
and call the same helpers (_find_bracket_end, _parse_bracket_keyword,
_parse_qbk, qbk_to_po, po_to_qbk) unchanged; ensure any test setup/assertions
that referenced self are adjusted to local variables and return values so tests
run under pytest/pytest-django.
🪄 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: 2dacccf5-978b-4ee6-815d-7c6f05a4cfd5
📒 Files selected for processing (4)
weblate/formats/tests/test_convert.pyweblate/trans/tests/data/cs.qbkweblate/trans/tests/data/cs2.qbkweblate/utils/tests/test_quickbook.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
weblate/formats/tests/test_convert.py (1)
395-503: ⚡ Quick winAdd one regression case for context-sensitive and fuzzy merges.
These tests only cover empty-context, non-fuzzy units, so the new
(source, context)andSTATE_FUZZYbranches in the QuickBook merge logic can still regress unnoticed. A fixture with duplicate source text in two contexts plus one fuzzyexisting_unitwould pin down the behavior this PR is changing.As per coding guidelines, "Write unit tests for new functionality and integration tests for complex features".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@weblate/formats/tests/test_convert.py` around lines 395 - 503, Add a regression test method to QuickBookFormatTest that covers context-sensitive and fuzzy merges: create a temp QuickBook file containing the same source string in two different contexts (e.g., two different [heading …] or sections), call the QuickBookFormat constructor with existing_units containing a MockUnit constructed with the duplicate source, the matching context for one occurrence, a fuzzy target and STATE_FUZZY, then call storage.save() and assert that only the occurrence with the matching context was updated with the fuzzy target and that the saved unit preserves the fuzzy state (use the same helpers as test_existing_units: MockUnit, STATE_FUZZY, storage.save(), Path(...).read_text/encoding checks). Ensure the new test method is named clearly (e.g., test_existing_units_context_and_fuzzy) and follows the same temp-file cleanup pattern as other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@weblate/formats/asciidoc.py`:
- Around line 58-69: The code only sets fuzzy on matched/created units but never
clears it, so ensure the fuzzy flag is synchronized both ways: when handling
lookups via store_units_index and when creating via store.addsourceunit (and
after thepo.setcontext/target assignment), check existing_unit.state against
STATE_FUZZY and call thepo.markfuzzy(True) if fuzzy or thepo.markfuzzy(False) if
not; update both branches that reference store_units_index, thepo,
existing_unit, STATE_FUZZY, markfuzzy, store.addsourceunit, setcontext and
target to keep the store unit’s fuzzy state identical to existing_unit.state.
In `@weblate/utils/quickbook.py`:
- Around line 863-867: The merge currently only sets fuzzy on the DB unit
(store_index -> po_unit) when ex_unit.state == STATE_FUZZY, so a stale fuzzy
flag can persist; update the block that handles existing keys (checking
store_index[key] and assigning po_unit.target) to explicitly clear the fuzzy
flag when ex_unit.state is not STATE_FUZZY by calling po_unit.markfuzzy(False)
(i.e., add an else branch to the existing if ex_unit.state == STATE_FUZZY
condition), ensuring po_unit reflects the extracted unit's fuzzy state.
- Around line 853-875: The merge creates duplicates because extracted units
store context as a developer note via unit.addnote(f"type: {seg.context}",
"developer") but the merge index keys on u.getcontext() (msgctxt), so contexts
appear empty; to fix, change the index-building logic that constructs
store_index to obtain context from the same representation used by extraction:
for each unit u use ctx = u.getcontext() or, if that is empty, read the
developer note that was set (the note string starting with "type: ") and extract
the context value, then use key = (u.source, ctx) so matching existing_units
(which use setcontext()/getcontext()) will find the right po_unit; update
references in the loop that builds store_index and keep references to
store_index, getcontext(), addnote(), setcontext(), existing_units,
store.addsourceunit, and new_unit.setcontext() in your change.
---
Nitpick comments:
In `@weblate/formats/tests/test_convert.py`:
- Around line 395-503: Add a regression test method to QuickBookFormatTest that
covers context-sensitive and fuzzy merges: create a temp QuickBook file
containing the same source string in two different contexts (e.g., two different
[heading …] or sections), call the QuickBookFormat constructor with
existing_units containing a MockUnit constructed with the duplicate source, the
matching context for one occurrence, a fuzzy target and STATE_FUZZY, then call
storage.save() and assert that only the occurrence with the matching context was
updated with the fuzzy target and that the saved unit preserves the fuzzy state
(use the same helpers as test_existing_units: MockUnit, STATE_FUZZY,
storage.save(), Path(...).read_text/encoding checks). Ensure the new test method
is named clearly (e.g., test_existing_units_context_and_fuzzy) and follows the
same temp-file cleanup pattern as other tests.
🪄 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: b0b4ba61-6aea-4c63-bebd-d8ad8886b65a
📒 Files selected for processing (4)
weblate/formats/asciidoc.pyweblate/formats/tests/test_convert.pyweblate/utils/quickbook.pyweblate/utils/tests/test_quickbook.py
🚧 Files skipped from review as they are similar to previous changes (1)
- weblate/utils/tests/test_quickbook.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
weblate/formats/quickbook.py (1)
48-135:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCI blocker:
convertfileexceeds the project's complexity budget (ruff C901: 17 > 16).The pre-commit pipeline is failing on this method. The new
if self.existing_units / elsebranch added at lines 95–102 pushed the cyclomatic complexity above the configured threshold. Extract one of the two large blocks (e.g., thestorefile_path == template_pathsource-language fill, or the translated-file positional-importelsebranch) into a private helper.♻️ Suggested refactor (extract the source-language fill)
+ def _fill_source_language_targets(self, store: TranslationStore) -> None: + """When loading the source-language file, populate empty unit targets with source.""" + merged = bool(self.existing_units) + for unit in store.units: + if unit.isheader(): + continue + if merged: + if not unit.target: + unit.target = unit.source + else: + unit.target = unit.source + def convertfile( self, storefile: IO[bytes], template_store: TranslationFormat | None, ) -> TranslationStore: ... storefile_path: str | None = getattr(storefile, "name", None) if storefile_path == template_path: - for unit in store.units: - if unit.isheader(): - continue - if self.existing_units: - if not unit.target: - unit.target = unit.source - else: - unit.target = unit.source + self._fill_source_language_targets(store) elif storefile_path is None: ...Alternatively, lift the translated-
.qbkpositional-import branch (lines 110–133) into_load_translated_qbk(storefile_path, store)— that block is self-contained and would reduce branches further.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@weblate/formats/quickbook.py` around lines 48 - 135, The method convertfile is over the complexity limit; extract the translated-file positional-import branch (the elif/else block that reads translated_content, builds translated_store, compares counts and assigns targets) into a private helper method named _load_translated_qbk(self, storefile_path: str, store: TranslationStore) -> None; move the try/except and all logic that reads Path(storefile_path).read_text, calls qbk_to_po, computes trans_units/tmpl_units, performs the length check/report_error, and assigns tmpl_unit.target from trans_unit.source into that helper, then replace the original else branch in convertfile with a single call to self._load_translated_qbk(storefile_path, store) so cyclomatic complexity drops.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@weblate/utils/quickbook.py`:
- Around line 870-895: The merge asymmetry comes from computing store keys with
_qbk_po_unit_merge_context(u) (which reads developer notes) while lookup uses
ex_unit.context directly; fix by making the lookup-side compute ctx from the
Weblate Unit's available fields (use ex_unit.context or, when empty, the Unit's
note/developer-note field exactly as the extractor stores it) before forming
key=(src, ctx), so existing_units matching uses the same context logic as
store_index; alternatively, standardize store-side to use u.getcontext() and
ensure the extractor calls setcontext(ctx) during extraction, but do not call
translate-toolkit-only methods on Weblate Unit objects (avoid
getcontext()/getnotes() on ex_unit).
---
Outside diff comments:
In `@weblate/formats/quickbook.py`:
- Around line 48-135: The method convertfile is over the complexity limit;
extract the translated-file positional-import branch (the elif/else block that
reads translated_content, builds translated_store, compares counts and assigns
targets) into a private helper method named _load_translated_qbk(self,
storefile_path: str, store: TranslationStore) -> None; move the try/except and
all logic that reads Path(storefile_path).read_text, calls qbk_to_po, computes
trans_units/tmpl_units, performs the length check/report_error, and assigns
tmpl_unit.target from trans_unit.source into that helper, then replace the
original else branch in convertfile with a single call to
self._load_translated_qbk(storefile_path, store) so cyclomatic complexity drops.
🪄 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: 91c8f4a0-65ff-462a-8697-ef112c89f1c2
📒 Files selected for processing (4)
weblate/formats/asciidoc.pyweblate/formats/quickbook.pyweblate/formats/tests/test_convert.pyweblate/utils/quickbook.py
🚧 Files skipped from review as they are similar to previous changes (1)
- weblate/formats/tests/test_convert.py
|
jonathanMLDev
approved these changes
May 8, 2026
wpak-ai
approved these changes
May 8, 2026
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
3 participants
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.
Closes #37
Summary by CodeRabbit