From 3d199cb783930cb70a6afc30a42646d947133c3a Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Fri, 10 Apr 2026 12:57:31 +0200 Subject: [PATCH] fix(clean-code): English mode strings, SRP extraction, rename getter - Replace French mode strings (configurer/verifier/preparer) with English equivalents (configure/verify/prepare) in StudioPage.vue and tests - Extract _build_conversion_options, _run_conversion, _finalize_analysis from _run_analysis_inner to respect Single Responsibility Principle - Rename _get_default_converter to _ensure_default_converter to reflect its lazy-init side effect Closes #136, closes #137, closes #138 --- document-parser/infra/local_converter.py | 4 +- document-parser/services/analysis_service.py | 114 ++++++++++-------- .../tests/test_pipeline_options.py | 18 +-- document-parser/tests/test_robustness.py | 4 +- .../src/features/history/navigation.test.ts | 14 +-- frontend/src/pages/StudioPage.vue | 30 ++--- 6 files changed, 101 insertions(+), 83 deletions(-) diff --git a/document-parser/infra/local_converter.py b/document-parser/infra/local_converter.py index 521b415..cb9db78 100644 --- a/document-parser/infra/local_converter.py +++ b/document-parser/infra/local_converter.py @@ -114,7 +114,7 @@ def _build_docling_converter(options: ConversionOptions) -> DoclingConverter: ) -def _get_default_converter() -> DoclingConverter: +def _ensure_default_converter() -> DoclingConverter: global _default_converter if _default_converter is None: try: @@ -127,7 +127,7 @@ def _get_default_converter() -> DoclingConverter: def _select_converter(options: ConversionOptions) -> DoclingConverter: if options.is_default(): - return _get_default_converter() + return _ensure_default_converter() return _build_docling_converter(options) diff --git a/document-parser/services/analysis_service.py b/document-parser/services/analysis_service.py index c0e18e5..b158272 100644 --- a/document-parser/services/analysis_service.py +++ b/document-parser/services/analysis_service.py @@ -268,6 +268,67 @@ class AnalysisService: job_id, file_path, filename, pipeline_options, chunking_options ) + def _build_conversion_options(self, pipeline_options: dict | None) -> ConversionOptions: + """Build ConversionOptions, applying default table_mode if not specified.""" + opts_dict = pipeline_options or {} + if "table_mode" not in opts_dict: + opts_dict = {**opts_dict, "table_mode": self._config.default_table_mode} + return ConversionOptions(**opts_dict) + + async def _run_conversion( + self, + job_id: str, + file_path: str, + options: ConversionOptions, + ) -> ConversionResult | None: + """Run batched or single conversion. Returns None if the job was deleted mid-batch.""" + total_pages = _count_pdf_pages(file_path) + batch_size = self._config.batch_page_size + + if batch_size > 0 and total_pages > batch_size: + return await self._run_batched_conversion( + job_id, file_path, options, total_pages, batch_size + ) + return await asyncio.wait_for( + self._converter.convert(file_path, options), + timeout=self._conversion_timeout, + ) + + async def _finalize_analysis( + self, + job_id: str, + result: ConversionResult, + chunking_options: dict | None, + ) -> None: + """Serialize results, optionally chunk, mark job completed, update page count.""" + pages_json = json.dumps([asdict(p) for p in result.pages]) + + chunks_json = None + if chunking_options and self._chunker and result.document_json: + chunk_opts = ChunkingOptions(**chunking_options) + chunks = await self._chunker.chunk(result.document_json, chunk_opts) + chunks_json = json.dumps([_chunk_to_dict(c) for c in chunks]) + logger.info("Chunking produced %d chunks for job %s", len(chunks), job_id) + + # Re-read the job so we don't lose progress_current/progress_total + # written to the DB during batched conversion. + job = await self._analysis_repo.find_by_id(job_id) + if not job: + return + job.mark_completed( + markdown=result.content_markdown, + html=result.content_html, + pages_json=pages_json, + document_json=result.document_json, + chunks_json=chunks_json, + ) + await self._analysis_repo.update_status(job) + + if result.page_count: + await self._document_repo.update_page_count(job.document_id, result.page_count) + + logger.info("Analysis completed: %s (%d pages)", job_id, result.page_count) + async def _run_analysis_inner( self, job_id: str, @@ -287,55 +348,12 @@ class AnalysisService: await self._analysis_repo.update_status(job) logger.info("Analysis started: %s (file: %s)", job_id, filename) - opts_dict = pipeline_options or {} - if "table_mode" not in opts_dict: - opts_dict = {**opts_dict, "table_mode": self._config.default_table_mode} - options = ConversionOptions(**opts_dict) + options = self._build_conversion_options(pipeline_options) + result = await self._run_conversion(job_id, file_path, options) + if result is None: + return # job was deleted mid-batch - total_pages = _count_pdf_pages(file_path) - batch_size = self._config.batch_page_size - - if batch_size > 0 and total_pages > batch_size: - result = await self._run_batched_conversion( - job_id, - file_path, - options, - total_pages, - batch_size, - ) - if result is None: - return # job was deleted mid-batch - else: - result = await asyncio.wait_for( - self._converter.convert(file_path, options), - timeout=self._conversion_timeout, - ) - - pages_json = json.dumps([asdict(p) for p in result.pages]) - - chunks_json = None - if chunking_options and self._chunker and result.document_json: - chunk_opts = ChunkingOptions(**chunking_options) - chunks = await self._chunker.chunk(result.document_json, chunk_opts) - chunks_json = json.dumps([_chunk_to_dict(c) for c in chunks]) - logger.info("Chunking produced %d chunks for job %s", len(chunks), job_id) - - # Re-read the job so we don't lose progress_current/progress_total - # written to the DB during batched conversion. - job = await self._analysis_repo.find_by_id(job_id) or job - job.mark_completed( - markdown=result.content_markdown, - html=result.content_html, - pages_json=pages_json, - document_json=result.document_json, - chunks_json=chunks_json, - ) - await self._analysis_repo.update_status(job) - - if result.page_count: - await self._document_repo.update_page_count(job.document_id, result.page_count) - - logger.info("Analysis completed: %s (%d pages)", job_id, result.page_count) + await self._finalize_analysis(job_id, result, chunking_options) except TimeoutError: logger.error("Analysis timed out after %ds: %s", self._conversion_timeout, job_id) diff --git a/document-parser/tests/test_pipeline_options.py b/document-parser/tests/test_pipeline_options.py index 3da6591..1b149ff 100644 --- a/document-parser/tests/test_pipeline_options.py +++ b/document-parser/tests/test_pipeline_options.py @@ -146,7 +146,7 @@ class TestBuildConverter: class TestConvertDocumentRouting: """Verify convert_document uses default converter for default opts, custom otherwise.""" - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_default_converter_with_all_defaults(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -164,7 +164,7 @@ class TestConvertDocumentRouting: mock_get_default.assert_called_once() mock_build.assert_not_called() - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_ocr_disabled(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -182,7 +182,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once() mock_get_default.assert_not_called() - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_table_mode_fast(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -200,7 +200,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once_with(opts) - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_code_enrichment_on(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -218,7 +218,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once_with(opts) - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_formula_enrichment_on(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -235,7 +235,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once() - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_picture_options_on(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -252,7 +252,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once() - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_generate_images_on(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -269,7 +269,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once() - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_uses_custom_converter_when_images_scale_changed(self, mock_build, mock_get_default): mock_conv = MagicMock() @@ -287,7 +287,7 @@ class TestConvertDocumentRouting: mock_build.assert_called_once_with(opts) - @patch("infra.local_converter._get_default_converter") + @patch("infra.local_converter._ensure_default_converter") @patch("infra.local_converter._build_docling_converter") def test_forwards_all_options_to_build_converter(self, mock_build, mock_get_default): mock_conv = MagicMock() diff --git a/document-parser/tests/test_robustness.py b/document-parser/tests/test_robustness.py index e650409..c828359 100644 --- a/document-parser/tests/test_robustness.py +++ b/document-parser/tests/test_robustness.py @@ -159,7 +159,7 @@ import infra.local_converter as lc_mod # noqa: E402 build_converter = lc_mod._build_docling_converter convert_sync = lc_mod._convert_sync -get_default_converter = lc_mod._get_default_converter +get_default_converter = lc_mod._ensure_default_converter # --------------------------------------------------------------------------- # C1 — document_timeout in PdfPipelineOptions @@ -317,7 +317,7 @@ class TestConvertSyncLimits: class TestGetDefaultConverterReset: - """Verify _get_default_converter resets on failure and retries on next call.""" + """Verify _ensure_default_converter resets on failure and retries on next call.""" @pytest.fixture(autouse=True) def reset_default_converter(self): diff --git a/frontend/src/features/history/navigation.test.ts b/frontend/src/features/history/navigation.test.ts index a5996a2..f1ba6af 100644 --- a/frontend/src/features/history/navigation.test.ts +++ b/frontend/src/features/history/navigation.test.ts @@ -125,7 +125,7 @@ describe('History → Studio navigation', () => { }) describe('Full restore flow (store-level integration)', () => { - it('restores completed analysis: selects analysis + document + verifier mode', async () => { + it('restores completed analysis: selects analysis + document + verify mode', async () => { const { fetchAnalysis } = await import('../analysis/api') fetchAnalysis.mockResolvedValue({ id: 'a1', @@ -147,12 +147,12 @@ describe('History → Studio navigation', () => { docStore.select(analysis.documentId) expect(docStore.selectedId).toBe('d1') - // Mode would be set to 'verifier' since status is COMPLETED - const mode = analysis.status === 'COMPLETED' ? 'verifier' : 'configurer' - expect(mode).toBe('verifier') + // Mode would be set to 'verify' since status is COMPLETED + const mode = analysis.status === 'COMPLETED' ? 'verify' : 'configure' + expect(mode).toBe('verify') }) - it('restores failed analysis: selects analysis + document, stays in configurer mode', async () => { + it('restores failed analysis: selects analysis + document, stays in configure mode', async () => { const { fetchAnalysis } = await import('../analysis/api') fetchAnalysis.mockResolvedValue({ id: 'a2', @@ -171,8 +171,8 @@ describe('History → Studio navigation', () => { docStore.select(analysis.documentId) expect(docStore.selectedId).toBe('d2') - const mode = analysis.status === 'COMPLETED' ? 'verifier' : 'configurer' - expect(mode).toBe('configurer') + const mode = analysis.status === 'COMPLETED' ? 'verify' : 'configure' + expect(mode).toBe('configure') }) it('handles missing analysis gracefully', async () => { diff --git a/frontend/src/pages/StudioPage.vue b/frontend/src/pages/StudioPage.vue index b0801c5..b92e037 100644 --- a/frontend/src/pages/StudioPage.vue +++ b/frontend/src/pages/StudioPage.vue @@ -23,8 +23,8 @@