Seguir la guia de Pydantic para suprimir una advertencia #182
Conversation
WalkthroughThis pull request addresses a Pydantic deprecation by relocating the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Areas requiring attention:
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clabe/validations.py(1 hunks)tests/test_clabe.py(2 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_clabe.pytests/test_types.pyclabe/validations.py
🧬 Code graph analysis (1)
tests/test_clabe.py (1)
clabe/validations.py (1)
add_bank(95-108)
🔇 Additional comments (1)
tests/test_types.py (1)
23-27: Review comment is based on incorrect assumptions about Pydantic behavior.The
json_schema_extrain theBankConfigRequestmodel is used only when generating JSON Schema and does not perform runtime validation. Runtime whitespace stripping in Pydantic requires theConfigDict(str_strip_whitespace=True)model configuration option, which is not present in this codebase.The test
test_valid_clabe_shows_no_warningscorrectly verifies that no warnings are emitted during CLABE validation and is unrelated to whitespace stripping. There is no actual whitespace stripping behavior being changed or tested here—the suggestion to add whitespace stripping tests is based on a misreading of the Pydantic Field configuration.Likely an incorrect or invalid review comment.
| bank_name: str = Field( | ||
| min_length=1, | ||
| strip_whitespace=True, | ||
| description="Bank name must have at least 1 character.", | ||
| json_schema_extra={ | ||
| "strip_whitespace": True, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
CRITICAL: Whitespace stripping behavior is broken.
Moving strip_whitespace to json_schema_extra only adds metadata to the JSON schema—it does not preserve the runtime whitespace stripping behavior that existed in Pydantic v1. The field will no longer strip whitespace from bank names during validation.
For Pydantic v2, use StringConstraints to preserve the behavior:
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, StringConstraints
+from typing import Annotated
class BankConfigRequest(BaseModel):
"""
Validates and processes bank configuration requests.
The class handles validation of bank names and codes, ensuring:
- Bank names are non-empty strings
- Banxico codes are exactly 5 digits
"""
- bank_name: str = Field(
+ bank_name: Annotated[str, StringConstraints(strip_whitespace=True)] = Field(
min_length=1,
description="Bank name must have at least 1 character.",
- json_schema_extra={
- "strip_whitespace": True,
- }
)This follows the Pydantic v2 migration guide and maintains the actual whitespace stripping functionality. As per coding guidelines.
Verify the current behavior with whitespace:
🏁 Script executed:
#!/bin/bash
# Description: Test if whitespace stripping still works
python3 << 'EOF'
from clabe.validations import BankConfigRequest
# Test with leading/trailing whitespace
try:
request = BankConfigRequest(
bank_code_banxico="90716",
bank_name=" Test Bank "
)
print(f"bank_name value: '{request.bank_name}'")
print(f"Whitespace stripped: {request.bank_name == 'Test Bank'}")
except Exception as e:
print(f"Error: {e}")
EOFLength of output: 59
CRITICAL: Whitespace stripping behavior is broken—verification confirms field retains spaces.
The test confirms that moving strip_whitespace to json_schema_extra does not preserve whitespace stripping. Input " Test Bank " remains unchanged instead of being stripped to "Test Bank". This violates the coding guideline requiring built-in Pydantic validators.
Fix at lines 77–83 using StringConstraints and Annotated:
from typing import Annotated
from pydantic import StringConstraints
bank_name: Annotated[str, StringConstraints(strip_whitespace=True)] = Field(
min_length=1,
description="Bank name must have at least 1 character.",
)This is the correct Pydantic v2 approach and will restore runtime whitespace stripping.
🤖 Prompt for AI Agents
In clabe/validations.py around lines 77–83, the Field currently uses
json_schema_extra to set strip_whitespace which does not perform runtime
stripping; replace that with Pydantic v2 string constraints by switching the
annotation to use typing.Annotated with
pydantic.StringConstraints(strip_whitespace=True) and remove strip_whitespace
from json_schema_extra so the bank_name field becomes an Annotated[str,
StringConstraints(strip_whitespace=True)] with the existing Field(min_length=1,
description=...) to restore runtime whitespace stripping.
There was a problem hiding this comment.
hola @FanGoH gracias por tu aporte! Observo algunos detalles, revisé la doc y en efecto esto suprime los warnings pero no corrige el issue principal que es hacer que el validator suprima espacios en blanco. Puedes revisar la sugerencia de @coderabbitai por favor?
There was a problem hiding this comment.
@felipao-mx Thank you for confirming! You're absolutely right - the current approach only suppresses the warning but loses the actual whitespace stripping behavior at runtime.
To summarize for @FanGoH:
- Current PR: Moves
strip_whitespace=Truetojson_schema_extra→ ❌ Breaks functionality (only adds schema metadata) - Correct fix: Use
Annotated[str, StringConstraints(strip_whitespace=True)]→ ✅ Preserves both functionality and Pydantic v2 compatibility
The suggested approach follows the official Pydantic v2 migration guide and will:
- ✅ Suppress the deprecation warning
- ✅ Maintain the whitespace stripping behavior
- ✅ Prepare for Pydantic v3 compatibility
Happy to help verify the fix once implemented! 🚀
| def test_add_bank_no_show_warnings(): | ||
| with warnings.catch_warnings(record=True) as caught_warnings: | ||
| warnings.simplefilter("always") | ||
| add_bank("90716", "Test bank") | ||
| assert len(caught_warnings) == 0, "Warnings were emitted during add_bank" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for whitespace stripping behavior.
While this test correctly verifies that no warnings are emitted, it doesn't validate the actual whitespace stripping behavior that was supposedly preserved from the Pydantic migration. Consider testing with whitespace in the bank name (e.g., add_bank("90716", " Test Bank ")) to ensure the behavior is maintained.
🤖 Prompt for AI Agents
In tests/test_clabe.py around lines 59 to 63, the test only asserts no warnings
but doesn't verify that add_bank strips surrounding whitespace from the bank
name; update the test to call add_bank("90716", " Test Bank ") (while still
using warnings.catch_warnings) and assert that the returned/recorded bank name
equals "Test Bank" (or lookup the stored bank entry and assert its name is
stripped), in addition to preserving the existing assertion that no warnings
were emitted.
2 participants
Ajustar la libreria para seguir las indicaciones en la guia de migracion de pydantic, esto permite suprimir un warning y preparar la compatibilidad para cuando se quiera utilizar pydantic v3
Se incluyen pruebas adicionales para verificar que los warnings ya no son emitidos
Summary by CodeRabbit