add expiration date field to VerificationDocument #125
Conversation
WalkthroughAdds two new properties to VerificationDocument in mati/types/enums.py: Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mati/types/enums.py(1 hunks)tests/test_types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
tests/test_types.pymati/types/enums.py
🧬 Code graph analysis (1)
tests/test_types.py (1)
mati/types/enums.py (3)
VerificationDocument(64-217)VerificationDocumentStep(48-52)expiration_date(188-195)
🔇 Additional comments (2)
tests/test_types.py (2)
5-5: LGTM!The import addition is correct. Using absolute imports from test files is acceptable per the coding guidelines, as test files are outside the main module structure.
49-73: Excellent test coverage.The test properly validates the new
expiration_dateproperty in both scenarios:
- When the field exists with a value
- When fields is None (edge case)
This ensures the property behaves correctly in all expected conditions.
rcabrera-py
force-pushed
the
feat/add-expiration-date
branch
from
2645342 to
891b8f4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 98.16% 98.25% +0.08%
==========================================
Files 12 12
Lines 382 401 +19
==========================================
+ Hits 375 394 +19
Misses 7 7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
rcabrera-py
force-pushed
the
feat/add-expiration-date
branch
3 times, most recently
from
5923e93 to
0bc7e15
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mati/types/enums.py(1 hunks)mati/version.py(1 hunks)tests/test_types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mati/types/enums.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
mati/version.pytests/test_types.py
🧬 Code graph analysis (1)
tests/test_types.py (1)
mati/types/enums.py (4)
VerificationDocument(64-227)VerificationDocumentStep(48-52)expiration_date(188-195)emission_date(198-205)
🔇 Additional comments (2)
mati/version.py (1)
1-1: LGTM!The development version bump is appropriate for the new feature additions (expiration_date and emission_date properties).
tests/test_types.py (1)
5-5: LGTM!The import addition is correct and necessary for the new tests.
rcabrera-py
force-pushed
the
feat/add-expiration-date
branch
from
0bc7e15 to
3441f77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_types.py (2)
79-106: Return type inconsistency with similar properties.The
emission_dateproperty has the same return type inconsistency asexpiration_date. It returnsNonefor missing/invalid fields while other similar properties return''.See the previous comment on
test_expiration_date_propertyfor details and suggested resolution.
79-106: Add test case for empty fields dict.Similar to
test_expiration_date_property, this test is missing the edge case for an empty fields dict{}.Add this test case to the parametrize list:
[ ( { 'emission_date': { 'value': '2023-01-15', 'label': 'Emission Date', 'format': 'date', } }, '2023-01-15', ), (None, None), ({'address': {'value': 'Test Address', 'label': 'Address'}}, None), ({'emission_date': {'label': 'Emission Date'}}, None), + ({}, None), ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mati/types/enums.py(1 hunks)mati/version.py(1 hunks)tests/test_types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mati/types/enums.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
mati/version.pytests/test_types.py
🧬 Code graph analysis (1)
tests/test_types.py (1)
mati/types/enums.py (4)
VerificationDocument(64-233)VerificationDocumentStep(48-52)expiration_date(188-198)emission_date(201-211)
🔇 Additional comments (3)
mati/version.py (1)
1-1: LGTM!The development version bump appropriately reflects the addition of new properties to the API.
tests/test_types.py (2)
5-5: LGTM!Absolute import is appropriate for test files located outside the main module structure.
49-76: Remove empty dict test suggestion.{}yields the same None result as the missing-expiration-date case already in your tests.Likely an incorrect or invalid review comment.
rcabrera-py
force-pushed
the
feat/add-expiration-date
branch
2 times, most recently
from
4b1f5f3 to
1db917f
Compare
rcabrera-py
force-pushed
the
feat/add-expiration-date
branch
from
1db917f to
6c14038
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
mati/types/enums.py (2)
188-203: Consider simplifying by removing the redundant None check.The explicit
if self.fields is None:check at line 197 is redundant because thetry/exceptblock already catchesTypeError, which would be raised when attempting to accessNone['expiration_date']. The coding guideline emphasizes the EAFP (Easier to Ask Forgiveness than Permission) idiom for accessing nested dictionary keys.While the current code is functionally correct and the explicit check does add some clarity, fully embracing EAFP would simplify the logic:
As per coding guidelines.
Apply this diff to simplify:
@property def expiration_date(self) -> Optional[date]: """ This property fills the expiration date direct from the ocr fields `expiration_date`. Returns a date object if the field is present and valid. Returns None if the field is missing, invalid, or fields is None. """ - if self.fields is None: - return None try: date_str = self.fields['expiration_date']['value'] return date.fromisoformat(date_str) except (KeyError, TypeError, ValueError): return None
205-220: Consider simplifying by removing the redundant None check.Similar to the
expiration_dateproperty, the explicitif self.fields is None:check is redundant here as well, sinceTypeErroris already caught by the exception handler.As per coding guidelines.
Apply this diff to simplify:
@property def emission_date(self) -> Optional[date]: """ This property fills the emission date direct from the ocr fields `emission_date`. Returns a date object if the field is present and valid. Returns None if the field is missing, invalid, or fields is None. """ - if self.fields is None: - return None try: date_str = self.fields['emission_date']['value'] return date.fromisoformat(date_str) except (KeyError, TypeError, ValueError): return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mati/types/enums.py(2 hunks)mati/version.py(1 hunks)tests/test_types.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- mati/version.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_types.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
mati/types/enums.py
🔇 Additional comments (1)
mati/types/enums.py (1)
2-2: LGTM!The datetime import is correctly added to support the new date-parsing properties.
rcabrera-py
force-pushed
the
feat/add-expiration-date
branch
from
6c14038 to
e86869d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mati/types/enums.py (1)
188-220: Well-implemented date properties.Both
expiration_dateandemission_dateproperties are correctly implemented with proper error handling, clear docstrings, and appropriate return type annotations. The use of try/except for nested dictionary access follows the coding guidelines, and catchingValueErrorensures invalid date formats are handled gracefully.Optional: Consider removing redundant None check.
The explicit
if self.fields is None: return Nonecheck is redundant sinceTypeErroris already caught by the except clause. Whenself.fieldsis None, accessingself.fields['expiration_date']raises a TypeError that's handled by the existing exception handler.You could simplify to:
@property def expiration_date(self) -> Optional[date]: """ This property fills the expiration date direct from the ocr fields `expiration_date`. - Returns a date object if the field is present and valid. - Returns None if the field is missing, invalid, or fields is None. + Returns a date object if the field is present and valid, + otherwise None. """ - if self.fields is None: - return None try: date_str = self.fields['expiration_date']['value'] return date.fromisoformat(date_str) except (KeyError, TypeError, ValueError): return NoneApply the same pattern to
emission_date:@property def emission_date(self) -> Optional[date]: """ This property fills the emission date direct from the ocr fields `emission_date`. - Returns a date object if the field is present and valid. - Returns None if the field is missing, invalid, or fields is None. + Returns a date object if the field is present and valid, + otherwise None. """ - if self.fields is None: - return None try: date_str = self.fields['emission_date']['value'] return date.fromisoformat(date_str) except (KeyError, TypeError, ValueError): return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mati/types/enums.py(2 hunks)mati/version.py(1 hunks)tests/test_types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mati/version.py
- tests/test_types.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
mati/types/enums.py
🔇 Additional comments (1)
mati/types/enums.py (1)
2-2: LGTM!The import is correctly placed and necessary for the new date-parsing properties.
Summary by CodeRabbit
New Features
Public API
Tests
Chores