Update/pydantic v2 #171
Update/pydantic v2 #171
Conversation
|
Warning Rate limit exceeded@gmorales96 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces changes to the CLABE validation library, focusing on upgrading to Pydantic v2 and updating Python version support. The modifications include a reduction in the Python version matrix in the GitHub Actions workflow, updating the Makefile to specify Python 3.8, and revising the README to reflect the new Python version requirement. The core library files see the removal of custom error classes and enhancements to the Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 5 -1
Lines 99 83 -16
=========================================
- Hits 99 83 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
![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
🧹 Nitpick comments (10)
clabe/validations.py (3)
66-91: Validate consistency and consider field validators
YourBankConfigRequestclass effectively enforces a minimum length and a numeric pattern for the bank code, and trims whitespace for the name. This usage is consistent with Pydantic v2. However, you might consider using Pydantic’s@field_validatoror@root_validatorfeatures to craft custom error messages or further refine logic if you foresee more sophisticated validation rules in the future.
93-107: Double-check concurrency and data overwriting
Theadd_bankfunction updates the in-memory dictionaries with no concurrency lock. If used in a multi-threaded scenario (e.g., a web API with multiple workers), these updates might cause data races. Also, adding an existing bank overwrites previous entries without warning, which might be desired or might warrant a safety check.
109-117: Non-existent bank removal
Theremove_bankfunction quietly ignores removal of unknown bank codes by passingNonetodict.pop. This is perfectly valid if silent failure is intended. If you want to confirm whether a removal actually took place, consider returning a boolean from this function to indicate success or failure.setup.py (1)
24-24: Open-ended Pydantic version
install_requires=['pydantic>=2.10.3']ensures compatibility with Pydantic v2. However, consider adding an upper bound like<3.0.0to avoid breaking changes from a future major release, unless you desire an open-ended version.tests/test_clabe.py (1)
45-56: Testing successful bank additions
This parameterized test confirms that valid codes and names are properly stored. Well done. One minor suggestion is to verify behavior when the same code is added again, to confirm overwriting or duplication logic is as intended.Makefile (1)
6-6: Consider updating black's target versionWhile the current setting is correct, consider updating black's target version for potential new Python syntax support.
-black = black -S -l 79 --target-version py38 $(PROJECT) tests setup.py +black = black -S -l 79 --target-version py39 $(PROJECT) tests setup.py.github/workflows/test.yml (1)
54-54: Consider updating GitHub Actions versionsThe workflow uses older versions of GitHub Actions (v2.1.0 for codecov, v2.2.1 for setup-python). Consider updating to the latest stable versions for improved features and security.
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - - uses: actions/[email protected] + - uses: actions/setup-python@v5 - uses: codecov/[email protected] + uses: codecov/codecov-action@v4README.md (1)
61-77: Documentation improvements needed for bank management API.While the new bank management API documentation is good, it could be enhanced with:
- Example error cases and handling
- Validation rules for bank codes
- Persistence behavior of added/removed banks
Consider adding these details to make the documentation more comprehensive:
## Agregar un nuevo banco A partir de la versión 2.0.0, el paquete se actualizará a **Pydantic v2**, lo que significa que las versiones anteriores ya no recibirán soporte. Sin embargo, hemos añadido una función para agregar bancos adicionales a la lista, en caso de que sea necesario. Esto se puede hacer sin necesidad de crear un PR. Para agregar un banco, simplemente llama a la siguiente función con el código de Banxico y el nombre del banco: ```python import clabe clabe.add_bank('12345', 'New Bank')
+### Validación
+El código del banco debe:
+- Ser una cadena de 5 dígitos
+- No existir previamente en la lista de bancos
+
+### Manejo de errores
+```python
+try:
- clabe.add_bank('invalid', 'New Bank')
+except ValueError as e:- print(f"Error: {e}") # Error: Bank code must be 5 digits
+```+### Persistencia
+Los bancos agregados/eliminados son válidos solo durante la ejecución del programa.
+Para persistir los cambios, considere [instrucciones de persistencia].Para eliminar un banco
import clabe clabe.remove_bank('12345')</blockquote></details> <details> <summary>clabe/types.py (2)</summary><blockquote> `18-21`: **Consider adding validation in __init__.** While the initialization looks correct, consider validating the bank codes during initialization to catch any inconsistencies between BANKS and BANK_NAMES dictionaries early. ```diff def __init__(self, clabe: str) -> None: + if clabe[:3] not in BANKS: + raise ValueError(f"Invalid bank code: {clabe[:3]}") self.bank_code_abm = clabe[:3] self.bank_code_banxico = BANKS[clabe[:3]] + if self.bank_code_banxico not in BANK_NAMES: + raise ValueError(f"Missing bank name for code: {self.bank_code_banxico}") self.bank_name = BANK_NAMES[self.bank_code_banxico]
58-69: Enhance error messages and validation.While the validation logic is correct, the error messages could be more descriptive and user-friendly.
def _validate(cls, clabe: str) -> 'Clabe': if not clabe.isdigit(): - raise PydanticCustomError('clabe', 'debe ser numérico') + raise PydanticCustomError( + 'clabe.format', + 'CLABE must contain only digits, got: {invalid_chars}', + {'invalid_chars': [c for c in clabe if not c.isdigit()]} + ) if clabe[:3] not in BANKS: raise PydanticCustomError( - 'clabe.bank_code', 'código de banco no es válido' + 'clabe.bank_code', + 'Invalid bank code: {code}. Must be one of: {valid_codes}', + {'code': clabe[:3], 'valid_codes': sorted(BANKS.keys())} ) if clabe[-1] != compute_control_digit(clabe): raise PydanticCustomError( - 'clabe.control_digit', 'clabe dígito de control no es válido' + 'clabe.control_digit', + 'Invalid control digit: {got}. Expected: {expected}', + {'got': clabe[-1], 'expected': compute_control_digit(clabe)} ) return cls(clabe)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/test.yml(2 hunks)Makefile(1 hunks)README.md(2 hunks)clabe/__init__.py(1 hunks)clabe/errors.py(0 hunks)clabe/types.py(2 hunks)clabe/validations.py(2 hunks)clabe/version.py(1 hunks)requirements-test.txt(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)tests/test_clabe.py(2 hunks)tests/test_types.py(2 hunks)
💤 Files with no reviewable changes (1)
- clabe/errors.py
✅ Files skipped from review due to trivial changes (2)
- requirements.txt
- clabe/version.py
🧰 Additional context used
🪛 LanguageTool
requirements-test.txt
[uncategorized] ~1-~1: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: pytest==7.4.4 pytest-cov==4.1.0 black==22.8.0 isort==5...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~2-~2: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: pytest==7.4.4 pytest-cov==4.1.0 black==22.8.0 isort==5.11.5 flake8==5.0....
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~3-~3: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...t==7.4.4 pytest-cov==4.1.0 black==22.8.0 isort==5.11.5 flake8==5.0.4 mypy==1.4.1
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~4-~4: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...t-cov==4.1.0 black==22.8.0 isort==5.11.5 flake8==5.0.4 mypy==1.4.1
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~5-~5: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...lack==22.8.0 isort==5.11.5 flake8==5.0.4 mypy==1.4.1
(AI_DE_GGEC_UNNECESSARY_SPACE)
🔇 Additional comments (15)
clabe/validations.py (1)
4-4: Use of pydantic library
The import of BaseModel and Field from Pydantic v2 is appropriate. This aligns with the PR objective to migrate to Pydantic v2 while preserving field-level validations.
clabe/__init__.py (1)
10-11: Re-exporting new functions
Exporting add_bank and remove_bank from __all__ clearly communicates their availability to users. This is a clean way to promote these functions as part of the public API.
Also applies to: 17-21
tests/test_types.py (3)
2-2: Pydantic-based testing
Switching to ValidationError from Pydantic to catch invalid data is aligned with the PR goal of fully migrating to Pydantic v2. This streamlined error handling is a solid approach.
22-50: Parameterized invalid CLABE tests
Using pytest.mark.parametrize to handle various invalid inputs (non-numeric, incorrect length, etc.) effectively verifies Pydantic’s validation. This coverage is thorough and well-organized.
53-65: Schema validation approach
The test_get_json_schema function confirms the model's schema matches expectations in detail. This is an excellent practice to ensure that changes to the model's fields or metadata are caught early.
tests/test_clabe.py (3)
4-5: Importing new bank management functions
The direct imports of BANK_NAMES, BANKS, add_bank, and remove_bank highlight that these functions are now first-class citizens in your public API, aligning with the rest of the CLABE library’s functionalities.
Also applies to: 10-10
58-70: Invalid input coverage
Your test_add_bank_invalid_inputs ensures robust coverage for erroneous cases, including too-short or too-long codes and empty names. This confirms that BankConfigRequest validation is working as intended.
72-84: Bank removal tests
Validating the removal of added banks and the no-op behavior for non-existent banks thoroughly covers the add/remove lifecycle. Excellent job.
Also applies to: 86-92
requirements-test.txt (1)
1-6: Verify compatibility with Pydantic v2
The updated test dependencies look good, but let's verify their compatibility with Pydantic v2 and Python versions.
✅ Verification successful
Let me gather more information about Pydantic compatibility, particularly focusing on mypy support.
Let me check the mypy configuration and Python version requirements.
Test dependencies are compatible with Pydantic v2 and Python requirements
Based on the verification:
- All test dependencies support Python >=3.7, which aligns with modern Python requirements
- The codebase uses Pydantic v2 features (evidenced by imports like
GetCoreSchemaHandler,GetJsonSchemaHandler, andpydantic_core) - Mypy 1.4.1 has built-in support for Pydantic v2 type checking
- The project has proper mypy configuration in setup.cfg
The selected versions are stable releases and compatible with the codebase's usage of Pydantic v2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any of these packages have known compatibility issues with Pydantic v2
packages=("pytest==7.4.4" "pytest-cov==4.1.0" "black==22.8.0" "isort==5.11.5" "flake8==5.0.4" "mypy==1.4.1")
for pkg in "${packages[@]}"; do
echo "Checking $pkg..."
# Check package metadata for Python version compatibility
curl -s "https://pypi.org/pypi/${pkg%%=*}/${pkg##*=}/json" | jq -r '.info.requires_python'
done
# Check if mypy has Pydantic v2 type stubs
curl -s "https://pypi.org/pypi/types-pydantic/json" | jq -r '.info.version'
Length of output: 1263
Script:
#!/bin/bash
# Check for Pydantic-related configurations and imports
rg -l "pydantic" --type py
# Check for mypy configuration
fd "mypy.ini|setup.cfg|pyproject.toml"
# Look for any existing Pydantic type annotations
rg "from pydantic import|from pydantic\." --type py -A 2
Length of output: 794
Script:
#!/bin/bash
# Check mypy configuration
cat setup.cfg
# Check Python version requirement
cat setup.py | grep -A 5 "python_requires"
Length of output: 366
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: pytest==7.4.4 pytest-cov==4.1.0 black==22.8.0 isort==5...
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~2-~2: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: pytest==7.4.4 pytest-cov==4.1.0 black==22.8.0 isort==5.11.5 flake8==5.0....
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~3-~3: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...t==7.4.4 pytest-cov==4.1.0 black==22.8.0 isort==5.11.5 flake8==5.0.4 mypy==1.4.1
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~4-~4: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...t-cov==4.1.0 black==22.8.0 isort==5.11.5 flake8==5.0.4 mypy==1.4.1
(AI_DE_GGEC_UNNECESSARY_SPACE)
[uncategorized] ~5-~5: Das sieht falsch platziert oder unnötig aus. Überlegen Sie, es zu löschen.
Context: ...lack==22.8.0 isort==5.11.5 flake8==5.0.4 mypy==1.4.1
(AI_DE_GGEC_UNNECESSARY_SPACE)
Makefile (1)
3-3: LGTM: Python version update aligns with Pydantic v2 requirements
The update to Python 3.8 is correct as it's the minimum version required for Pydantic v2.
README.md (1)
13-13: LGTM: Python version requirement updated correctly.
The Python version requirement has been updated to 3.8+ which aligns with the PR objectives of dropping Python 3.7 support.
clabe/types.py (4)
1-4: LGTM: Clean import structure for Pydantic v2.
The imports have been correctly updated to use the new Pydantic v2 API components.
23-25: LGTM: Clean property implementation.
The bank_code property provides a clean way to access the Banxico code.
28-40: LGTM: Well-structured JSON schema implementation.
The JSON schema implementation follows Pydantic v2 best practices with good examples and clear type constraints.
43-55: LGTM: Proper core schema implementation.
The core schema implementation correctly uses the new Pydantic v2 API with appropriate validation constraints.
- Updated Python version from 3.7 to 3.8 in Makefile and README.md. - Upgraded Pydantic dependency from 1.9.0 to 2.10.3 in requirements.txt and setup.py. - Updated mypy version from 0.790 to 1.13.0 in requirements-test.txt. - Refactored CLABE validation logic in types.py, removing custom error classes and integrating Pydantic's validation features. - Removed unused error handling code and updated tests to reflect changes in validation logic. - Updated GitHub Actions workflow to support Python versions 3.8 through 3.13. - Bumped version to 2.0.0.dev0 in version.py.
* Add python version actions * Update test.yml * update test deps --------- Co-authored-by: Felipe López <[email protected]>
- Updated Python version from 3.7 to 3.8 in Makefile and README.md. - Upgraded Pydantic dependency from 1.9.0 to 2.10.3 in requirements.txt and setup.py. - Updated mypy version from 0.790 to 1.13.0 in requirements-test.txt. - Refactored CLABE validation logic in types.py, removing custom error classes and integrating Pydantic's validation features. - Removed unused error handling code and updated tests to reflect changes in validation logic. - Updated GitHub Actions workflow to support Python versions 3.8 through 3.13. - Bumped version to 2.0.0.dev0 in version.py.
…matrix, supporting versions 3.8 through 3.13.
gmorales96
force-pushed
the
update/pydantic-v2
branch
from
1c8a69c to
b4cbfb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
clabe/types.py (2)
18-25: Consider adding validation in initWhile the property and initialization look correct, consider adding validation in
__init__to ensure the class invariants are maintained even when instantiated directly without Pydantic.def __init__(self, clabe: str) -> None: + if not isinstance(clabe, str): + raise TypeError("CLABE must be a string") + if len(clabe) != self.max_length: + raise ValueError(f"CLABE must be {self.max_length} characters long") self.bank_code_abm = clabe[:3] self.bank_code_banxico = BANKS[clabe[:3]] self.bank_name = BANK_NAMES[self.bank_code_banxico]
58-69: Consider enhancing error messagesThe validation logic is correct, but error messages could be more descriptive to help users fix issues.
def _validate(cls, clabe: str) -> 'Clabe': if not clabe.isdigit(): - raise PydanticCustomError('clabe', 'debe ser numérico') + raise PydanticCustomError('clabe', 'CLABE debe contener solo dígitos numéricos') if clabe[:3] not in BANKS: raise PydanticCustomError( - 'clabe.bank_code', 'código de banco no es válido' + 'clabe.bank_code', f'código de banco {clabe[:3]} no es válido' ) if clabe[-1] != compute_control_digit(clabe): raise PydanticCustomError( - 'clabe.control_digit', 'clabe dígito de control no es válido' + 'clabe.control_digit', f'dígito de control {clabe[-1]} no es válido para CLABE {clabe}' ) return cls(clabe)tests/test_types.py (1)
53-65: Consider adding more schema test casesWhile the basic schema test is good, consider adding more test cases to verify schema behavior.
def test_get_json_schema_validation() -> None: from pydantic import TypeAdapter adapter = TypeAdapter(Clabe) # Test invalid length with pytest.raises(ValidationError): adapter.validate_python("1" * 17) # Test non-numeric with pytest.raises(ValidationError): adapter.validate_python("A" * 18) # Test valid input result = adapter.validate_python("002000000000000008") assert isinstance(result, Clabe)README.md (1)
Line range hint
61-77: Consider enhancing documentation with version compatibility notesWhile the Pydantic v2 upgrade notice is good, consider adding more details about version compatibility and migration.
Add a section about version compatibility:
### Compatibilidad de versiones - Versión 1.x: Compatible con Pydantic v1 y Python 3.7+ - Versión 2.x: Compatible con Pydantic v2 y Python 3.8+ Para migrar de la versión 1.x a 2.x, asegúrate de: 1. Actualizar a Python 3.8 o superior 2. Actualizar Pydantic a la versión 2.x 3. Revisar los cambios en la validación de CLABE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks)Makefile(1 hunks)README.md(2 hunks)clabe/errors.py(0 hunks)clabe/types.py(2 hunks)clabe/validations.py(1 hunks)clabe/version.py(1 hunks)requirements-test.txt(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)tests/test_types.py(2 hunks)
💤 Files with no reviewable changes (1)
- clabe/errors.py
🚧 Files skipped from review as they are similar to previous changes (7)
- requirements.txt
- Makefile
- clabe/version.py
- .github/workflows/test.yml
- setup.py
- clabe/validations.py
- requirements-test.txt
🔇 Additional comments (4)
clabe/types.py (3)
1-4: LGTM: Import changes align with Pydantic v2
The imports have been correctly updated to use the new Pydantic v2 components for schema handling and custom errors.
28-40: LGTM: JSON schema generation follows Pydantic v2 patterns
The __get_pydantic_json_schema__ implementation correctly provides schema metadata including pattern validation and examples.
43-55: LGTM: Core schema setup aligns with Pydantic v2
The __get_pydantic_core_schema__ correctly sets up string validation with length constraints and whitespace handling.
tests/test_types.py (1)
22-50: LGTM: Comprehensive test coverage for invalid cases
The parameterized tests effectively cover various validation scenarios including non-numeric input, length validation, and invalid bank codes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
setup.py (1)
24-25: Consider adding an upper bound to the Pydantic version requirementWhile the migration to Pydantic v2 and Python 3.8+ looks good, consider adding an upper bound to the Pydantic requirement to prevent potential compatibility issues with future major versions:
- install_requires=['pydantic>=2.10.3'], + install_requires=['pydantic>=2.10.3,<3.0.0'],clabe/types.py (1)
56-67: Consider standardizing error messages to EnglishThe validation error messages are in Spanish. Consider standardizing them to English for consistency with other error messages in the codebase:
if not clabe.isdigit(): - raise PydanticCustomError('clabe', 'debe ser numérico') + raise PydanticCustomError('clabe', 'must be numeric') if clabe[:3] not in BANKS: raise PydanticCustomError( - 'clabe.bank_code', 'código de banco no es válido' + 'clabe.bank_code', 'invalid bank code' ) if clabe[-1] != compute_control_digit(clabe): raise PydanticCustomError( - 'clabe.control_digit', 'clabe dígito de control no es válido' + 'clabe.control_digit', 'invalid control digit' )README.md (2)
33-62: Great addition of the Pydantic integration example!The example effectively demonstrates how to use the
Clabetype in Pydantic models, including both successful and error cases. This addresses the previous request for custom type usage documentation.Add language specification to the code block:
-``` +```python🧰 Tools
🪛 Markdownlint (0.37.0)
35-35: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
92-116: Polish the bank management section formatting and clarity.The content effectively communicates the changes and maintenance policy. However, there are some formatting improvements needed:
- Add space after hash in headers:
-###Cómo eliminar un banco +### Cómo eliminar un banco
- Consider these language improvements:
- Line 96: Replace "PRs" with "pull requests" for formal documentation
- Line 100: Add a comma after "banco" in Spanish text
The section successfully addresses the previous comment about bank list updates by clarifying the maintenance policy for version 2+.
🧰 Tools
🪛 LanguageTool
[misspelling] ~96-~96: El plural de las siglas no se marca gráficamente.
Context: ...ndo la lista oficial de bancos mediante PRs en el repositorio. ### Cómo agregar un...(SIGLAS)
[uncategorized] ~100-~100: Probablemente hay un error. Considere aplicar la sugerencia.
Context: ...ra agregar un banco, llama a la funciónadd_bankpasando el código de Banxico y...(AI_ES_GGEC_REPLACEMENT_PUNCTUATION)
🪛 Markdownlint (0.37.0)
107-107: null
No space after hash on atx style heading(MD018, no-missing-space-atx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)clabe/types.py(1 hunks)clabe/version.py(1 hunks)setup.py(1 hunks)tests/test_types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clabe/version.py
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~96-~96: El plural de las siglas no se marca gráficamente.
Context: ...ndo la lista oficial de bancos mediante PRs en el repositorio. ### Cómo agregar un...
(SIGLAS)
[uncategorized] ~100-~100: Probablemente hay un error. Considere aplicar la sugerencia.
Context: ...ra agregar un banco, llama a la función add_bank pasando el código de Banxico y...
(AI_ES_GGEC_REPLACEMENT_PUNCTUATION)
🪛 Markdownlint (0.37.0)
README.md
107-107: null
No space after hash on atx style heading
(MD018, no-missing-space-atx)
35-35: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
clabe/types.py (3)
8-8: Good use of constant for CLABE length
The CLABE_LENGTH constant improves code maintainability and follows DRY principles.
26-38: Well-structured JSON schema generation
The JSON schema generation is well-implemented with proper type, pattern, description, and examples.
41-53: Consider removing redundant length validation parameters
As mentioned in previous reviews, the min_length, max_length, and strip_whitespace parameters are no longer necessary in Pydantic v2. You can simplify this by using the constant directly:
return core_schema.no_info_after_validator_function(
cls._validate,
core_schema.str_schema(
- min_length=CLABE_LENGTH,
- max_length=CLABE_LENGTH,
- strip_whitespace=True,
+ pattern=f"^[0-9]{{{CLABE_LENGTH}}}$",
),
)tests/test_types.py (3)
22-56: Add missing test case for longer CLABE
The test cases look good, but as suggested in previous reviews, consider adding a test case for CLABE longer than 18 characters:
[
# ... existing test cases ...
+ pytest.param(
+ '1234567890123456789', # 19 digits
+ 'String should have at most 18 characters',
+ id='clabe_too_long',
+ ),
],63-75: Well-structured JSON schema test
The JSON schema test is well-implemented and covers all necessary schema attributes.
7-7: Consider using a Cuenca CLABE example
As suggested in previous reviews, consider using a Cuenca CLABE as the example:
README.md (1)
13-13: LGTM: Version requirement update is clear and accurate.
The updated Python version requirement aligns with the migration to Pydantic v2.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
33-61: Add brief explanation of CLABE validation rules.The Pydantic integration example is comprehensive and addresses the previous request for custom type documentation. Consider adding a brief note explaining what makes a CLABE number valid/invalid to provide more context for the validation error example.
### Como tipo personalizado en un modelo de Pydantic +Un número CLABE válido debe cumplir con las siguientes reglas: +- Longitud de 18 dígitos +- Código de banco válido (primeros 3 dígitos) +- Dígito de control correcto (último dígito) + ```python from pydantic import BaseModel, ValidationError
106-106: Fix heading formatting.Add a space after the hash symbol in the heading.
-###Cómo eliminar un banco +### Cómo eliminar un banco🧰 Tools
🪛 Markdownlint (0.37.0)
106-106: null
No space after hash on atx style heading(MD018, no-missing-space-atx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[misspelling] ~95-~95: El plural de las siglas no se marca gráficamente.
Context: ...ndo la lista oficial de bancos mediante PRs en el repositorio. ### Cómo agregar un...
(SIGLAS)
[uncategorized] ~99-~99: Probablemente hay un error. Considere aplicar la sugerencia.
Context: ...ra agregar un banco, llama a la función add_bank pasando el código de Banxico y...
(AI_ES_GGEC_REPLACEMENT_PUNCTUATION)
🪛 Markdownlint (0.37.0)
README.md
106-106: null
No space after hash on atx style heading
(MD018, no-missing-space-atx)
🔇 Additional comments (2)
README.md (2)
13-13: LGTM: Version requirement update is clear and consistent.
The updated Python version requirement aligns with the PR objective of dropping Python 3.7 support.
91-115: LGTM: Clear documentation of bank management features.
The section effectively communicates:
- The migration to Pydantic v2
- The new bank management functions
- The policy for maintaining the official bank list
This addresses the previous feedback about clarifying the bank list update policy.
🧰 Tools
🪛 LanguageTool
[misspelling] ~95-~95: El plural de las siglas no se marca gráficamente.
Context: ...ndo la lista oficial de bancos mediante PRs en el repositorio. ### Cómo agregar un...
(SIGLAS)
[uncategorized] ~99-~99: Probablemente hay un error. Considere aplicar la sugerencia.
Context: ...ra agregar un banco, llama a la función add_bank pasando el código de Banxico y...
(AI_ES_GGEC_REPLACEMENT_PUNCTUATION)
🪛 Markdownlint (0.37.0)
106-106: null
No space after hash on atx style heading
(MD018, no-missing-space-atx)
Description:
This PR migrates the project to Pydantic v2 and drops support for Python 3.7
Key Changes:
Breaking Changes:
Summary by CodeRabbit
New Features
Bug Fixes
BankConfigRequestclass.Documentation
Chores
Versioning