fix: improvments for docs and warning#100
Conversation
Confidence Score: 5/5Safe to merge; the one remaining finding is a minor style suggestion with no runtime impact. All changes are documentation updates, UX improvements (msgprint warning), and error-message enhancements. The single comment is P2 (style) — adding a transaction_parser/transaction_parser/utils/pdf_processor.py — minor style fix in
|
| Filename | Overview |
|---|---|
| README.md | Adds Anthropic/claude-haiku-4-5 and gpt-5-mini to model lists, corrects Google Gemini model name formatting, and updates the API key table and model comparison table accordingly. |
| transaction_parser/transaction_parser/doctype/transaction_parser_settings/transaction_parser_settings.py | Adds warn_on_pdf_processor_change() to show an orange msgprint with a setup-docs link whenever the pdf_processor field is changed; implementation is correct. |
| transaction_parser/transaction_parser/utils/pdf_processor.py | Adds SETUP_URL class attributes to all three processors, updates ImportError messages with setup links, and wraps the bare imports in _get_converter() with a try/except — but leaves the code that uses the imported names outside the try block, causing a latent NameError risk under test mocking. |
Reviews (1): Last reviewed commit: "fix: add warning for PDF processor chang..." | Re-trigger Greptile
📝 WalkthroughWalkthroughThis pull request adds documentation for new AI models (Anthropic Claude Haiku-4.5 and OpenAI gpt-5-mini) and updates model naming conventions. A validation mechanism is introduced to warn users when PDF processor settings change, directing them to verify required dependencies are installed. Missing dependency error messages are enhanced with setup instruction hyperlinks for each PDF processor type. Import error handling for the Docling PDF processor is improved with explicit exception catching. 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
transaction_parser/transaction_parser/doctype/transaction_parser_settings/transaction_parser_settings.py (1)
72-79: Use processor-specific setup links instead of one generic anchor.
pdf_processor.pynow has per-processorSETUP_URLvalues, but this warning always points to a generic section. Resolving by selected processor will keep links precise and avoid drift.♻️ Proposed refactor
+from transaction_parser.transaction_parser.utils.pdf_processor import BUILTIN_PDF_PROCESSORS ... def warn_on_pdf_processor_change(self): if not self.has_value_changed("pdf_processor"): return + setup_url = "https://github.com/resilient-tech/transaction-parser#pdf-processor-setup" + class_path = BUILTIN_PDF_PROCESSORS.get(self.pdf_processor) + if class_path: + processor_cls = frappe.get_attr(class_path) + setup_url = getattr(processor_cls, "SETUP_URL", setup_url) frappe.msgprint( _( "Make sure the required dependencies for {0} are installed.<br>" "See {1} for setup instructions." ).format( frappe.bold(self.pdf_processor), - '<a href="https://github.com/resilient-tech/transaction-parser#pdf-processor-setup" target="_blank">PDF Processor Setup</a>', + f'<a href="{setup_url}" target="_blank">PDF Processor Setup</a>', ), title=_("PDF Processor Changed"), indicator="orange", )transaction_parser/transaction_parser/utils/pdf_processor.py (1)
110-112: Extract duplicated missing-dependency message formatting.The same HTML/template pattern is repeated across processors. A shared helper will reduce drift and simplify future updates.
♻️ Proposed refactor
class PDFProcessor(ABC): + def throw_missing_dependency(self, package: str, install_cmd: str): + setup_url = getattr(self, "SETUP_URL", "https://github.com/resilient-tech/transaction-parser#pdf-processor-setup") + frappe.throw( + title=_("Missing Dependency"), + msg=_( + "{0} is not installed.<br>" + "Install it with: <code>{1}</code><br>" + "See <a href='{2}'>setup instructions</a> for more details." + ).format(package, install_cmd, setup_url), + ) class DoclingPDFProcessor(PDFProcessor): ... except ImportError: - frappe.throw( - title=_("Missing Dependency"), - msg=_( - "docling is not installed.<br>" - "Install it with: <code>bench pip install transaction_parser[docling]</code><br>" - "See <a href='{0}'>setup instructions</a> for more details." - ).format(self.SETUP_URL), - ) + self.throw_missing_dependency( + "docling", "bench pip install transaction_parser[docling]" + )Apply similarly in
DoclingPDFProcessor._get_converter(),PDFtoTextProcessor.get_text(), andOCRMyPDFProcessor.apply_ocr().Also applies to: 153-156, 196-197, 229-230
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80f50e81-2f9d-484b-9dd2-e89718ac25d2
📒 Files selected for processing (3)
README.mdtransaction_parser/transaction_parser/doctype/transaction_parser_settings/transaction_parser_settings.pytransaction_parser/transaction_parser/utils/pdf_processor.py
No description provided.