Add configure_additional_bank Functionality and Bump Version to 1.2.17 #169
Conversation
…-mx/clabe-python into feature/add-editable-banks-config
WalkthroughThis pull request introduces new functionality to the CLABE library for managing bank information. The 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
clabe/errors.py (1)
14-21: Consider enhancing error messages with dynamic values.The error messages could be more helpful by including the conflicting bank code in the message.
class BankCodeABMAlreadyExistsError(PydanticValueError): code = 'clabe.bank_code_abm_already_exists' - msg_template = 'código de banco ABM ya existe' + msg_template = 'código de banco ABM {code} ya existe' class BankCodeBanxicoAlreadyExistsError(PydanticValueError): code = 'clabe.bank_code_banxico_already_exists' - msg_template = 'código de banco banxico ya existe' + msg_template = 'código de banco banxico {code} ya existe'README.md (1)
61-66: Enhance documentation with parameter descriptions and validation rules.While the example is clear, it would be more helpful to:
- Document the parameter requirements and constraints
- Explain the validation rules
- Show error handling examples
Consider expanding the example like this:
Para generar un nuevo banco +# Parameters: +# - bank_code_abm: 3-digit code assigned by ABM (e.g., '777') +# - bank_code_banxico: 3-digit code assigned by Banxico (e.g., '713') +# - bank_name: Official bank name + +# Example: Adding a new bank ```python import clabe -clabe.configure_additional_bank('777', '713', 'New Bank') +try: + clabe.configure_additional_bank( + bank_code_abm='777', # ABM assigned code + bank_code_banxico='713', # Banxico assigned code + bank_name='New Bank' # Official bank name + ) +except clabe.BankCodeABMAlreadyExistsError: + print("Bank with this ABM code already exists") +except clabe.BankCodeBanxicoAlreadyExistsError: + print("Bank with this Banxico code already exists")</blockquote></details> <details> <summary>clabe/validations.py (1)</summary><blockquote> `70-84`: **Update docstring to reflect actual exceptions.** The docstring mentions `ValueError` but the function raises custom exceptions `BankCodeABMAlreadyExistsError` and `BankCodeBanxicoAlreadyExistsError`. ```diff """ Configures an additional bank. Args: bank_code_abm (str): The ABM code for the bank. bank_code_banxico (str): The Banxico code for the bank. bank_name (str): The name of the bank. Raises: - ValueError: If the bank_code_abm or bank_code_banxico - already exists in the provided dictionaries. + BankCodeABMAlreadyExistsError: If the bank_code_abm already exists + BankCodeBanxicoAlreadyExistsError: If the bank_code_banxico already exists + ValueError: If the input parameters have invalid format """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)clabe/__init__.py(1 hunks)clabe/errors.py(1 hunks)clabe/validations.py(2 hunks)clabe/version.py(1 hunks)tests/test_clabe.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- clabe/version.py
🔇 Additional comments (5)
clabe/__init__.py (1)
10-10: LGTM! Export and import of configure_additional_bank are properly implemented.
The new function is correctly added to __all__ and imported from the validations module.
Also applies to: 17-17
clabe/errors.py (1)
14-16: LGTM! Error class implementation follows the established patterns.
The error classes correctly inherit from PydanticValueError and maintain consistency with existing error implementations.
tests/test_clabe.py (1)
5-5: LGTM: Import statements are correctly organized.
The new imports for configure_additional_bank and its associated error classes are properly added.
Also applies to: 10-13
clabe/validations.py (2)
5-8: LGTM: Error imports are properly organized.
The new error class imports are correctly added.
70-94: Verify thread safety across the codebase.
The modification of global dictionaries BANKS and BANK_NAMES could lead to race conditions. Let's verify their usage across the codebase.
clabe/validations.py
Outdated
| if bank_code_abm in BANKS: | ||
| raise BankCodeABMAlreadyExistsError | ||
|
|
||
| if bank_code_banxico in BANK_NAMES: | ||
| raise BankCodeBanxicoAlreadyExistsError | ||
|
|
||
| BANKS.update({bank_code_abm: bank_code_banxico}) | ||
|
|
||
| BANK_NAMES.update({bank_code_banxico: bank_name}) |
There was a problem hiding this comment.
Add input validation and thread safety.
The function needs additional validation and safety measures:
- Missing validation for bank code format and length
- No type checking despite type hints
- Global state modification without thread safety
Consider this implementation:
+import threading
+from typing import Pattern
+import re
+
+# Constants
+BANK_CODE_ABM_PATTERN: Pattern = re.compile(r'^\d{3}$')
+BANK_CODE_BANXICO_PATTERN: Pattern = re.compile(r'^\d{5}$')
+_bank_lock = threading.Lock()
def configure_additional_bank(
bank_code_abm: str, bank_code_banxico: str, bank_name: str
) -> None:
+ # Type validation
+ if not all(isinstance(x, str) for x in [bank_code_abm, bank_code_banxico, bank_name]):
+ raise TypeError("All parameters must be strings")
+
+ # Format validation
+ if not BANK_CODE_ABM_PATTERN.match(bank_code_abm):
+ raise ValueError("bank_code_abm must be a 3-digit number")
+ if not BANK_CODE_BANXICO_PATTERN.match(bank_code_banxico):
+ raise ValueError("bank_code_banxico must be a 5-digit number")
+ if not bank_name.strip():
+ raise ValueError("bank_name cannot be empty")
- if bank_code_abm in BANKS:
- raise BankCodeABMAlreadyExistsError
+ with _bank_lock:
+ if bank_code_abm in BANKS:
+ raise BankCodeABMAlreadyExistsError
- if bank_code_banxico in BANK_NAMES:
- raise BankCodeBanxicoAlreadyExistsError
+ if bank_code_banxico in BANK_NAMES:
+ raise BankCodeBanxicoAlreadyExistsError
- BANKS.update({bank_code_abm: bank_code_banxico})
- BANK_NAMES.update({bank_code_banxico: bank_name})
+ BANKS[bank_code_abm] = bank_code_banxico
+ BANK_NAMES[bank_code_banxico] = bank_name.strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if bank_code_abm in BANKS: | |
| raise BankCodeABMAlreadyExistsError | |
| if bank_code_banxico in BANK_NAMES: | |
| raise BankCodeBanxicoAlreadyExistsError | |
| BANKS.update({bank_code_abm: bank_code_banxico}) | |
| BANK_NAMES.update({bank_code_banxico: bank_name}) | |
| import threading | |
| from typing import Pattern | |
| import re | |
| # Constants | |
| BANK_CODE_ABM_PATTERN: Pattern = re.compile(r'^\d{3}$') | |
| BANK_CODE_BANXICO_PATTERN: Pattern = re.compile(r'^\d{5}$') | |
| _bank_lock = threading.Lock() | |
| def configure_additional_bank( | |
| bank_code_abm: str, bank_code_banxico: str, bank_name: str | |
| ) -> None: | |
| # Type validation | |
| if not all(isinstance(x, str) for x in [bank_code_abm, bank_code_banxico, bank_name]): | |
| raise TypeError("All parameters must be strings") | |
| # Format validation | |
| if not BANK_CODE_ABM_PATTERN.match(bank_code_abm): | |
| raise ValueError("bank_code_abm must be a 3-digit number") | |
| if not BANK_CODE_BANXICO_PATTERN.match(bank_code_banxico): | |
| raise ValueError("bank_code_banxico must be a 5-digit number") | |
| if not bank_name.strip(): | |
| raise ValueError("bank_name cannot be empty") | |
| with _bank_lock: | |
| if bank_code_abm in BANKS: | |
| raise BankCodeABMAlreadyExistsError | |
| if bank_code_banxico in BANK_NAMES: | |
| raise BankCodeBanxicoAlreadyExistsError | |
| BANKS[bank_code_abm] = bank_code_banxico | |
| BANK_NAMES[bank_code_banxico] = bank_name.strip() |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 85 99 +14
=========================================
+ Hits 85 99 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_clabe.py (1)
61-71: Add more edge cases for invalid inputs.While the invalid input tests are good, consider adding these additional cases:
- Bank codes with incorrect length (2 digits, 4 digits)
- Bank name with only whitespace
- Unicode characters in bank codes
def test_configure_additional_bank_more_edge_cases(): with pytest.raises(ValueError): configure_additional_bank("12", "40137", "Short Code") # 2 digits with pytest.raises(ValueError): configure_additional_bank("1234", "40137", "Long Code") # 4 digits with pytest.raises(ValueError): configure_additional_bank("123", "40137", " ") # only whitespace with pytest.raises(ValueError): configure_additional_bank("12３", "40137", "Unicode") # unicode digitclabe/validations.py (1)
75-86: Enhance docstring with validation details.The docstring should document the validation rules and all possible exceptions.
def configure_additional_bank( bank_code_abm: str, bank_code_banxico: str, bank_name: str ) -> None: """ Configures an additional bank. Args: bank_code_abm (str): The ABM code for the bank. bank_code_banxico (str): The Banxico code for the bank. bank_name (str): The name of the bank. Raises: - ValueError: If the bank_code_abm or bank_code_banxico - already exists in the provided dictionaries. + TypeError: If any parameter is not a string + ValueError: If bank_code_abm is not exactly 3 digits + ValueError: If bank_code_banxico is not exactly 5 digits + ValueError: If bank_name is empty or only whitespace + NotDigitError: If bank codes contain non-digit characters + BankCodeABMAlreadyExistsError: If bank_code_abm already exists + BankCodeBanxicoAlreadyExistsError: If bank_code_banxico already exists """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clabe/validations.py(2 hunks)tests/test_clabe.py(2 hunks)
🔇 Additional comments (5)
tests/test_clabe.py (3)
5-5: LGTM! Imports are correctly organized.
The new imports for configure_additional_bank and error classes are properly added and organized.
Also applies to: 10-13
46-48: Add test cleanup to prevent side effects.
The test modifies global state (BANKS and BANK_NAMES dictionaries) but doesn't clean up after itself. This could affect other tests.
51-58: Use test-specific bank codes instead of production values.
The tests use production bank codes ("002", "40137") which could become invalid if bank codes change. Consider using test-specific codes instead.
clabe/validations.py (2)
4-4: LGTM! Imports are properly organized.
The new imports for NotDigitError and custom error classes are correctly added.
Also applies to: 7-10
72-110:
Add thread safety and enhance input validation.
While the function implementation is generally good, it needs additional safety measures:
- Missing thread safety for global state modification
- Missing validation for bank code length
- Using direct dictionary updates without atomic operations
Consider this enhanced implementation:
+import threading
+
+# Thread safety
+_bank_lock = threading.Lock()
def configure_additional_bank(
bank_code_abm: str, bank_code_banxico: str, bank_name: str
) -> None:
if not all(isinstance(x, str) for x in [bank_code_abm, bank_code_banxico, bank_name]):
raise TypeError("All parameters must be strings")
+ # Validate lengths
+ if len(bank_code_abm) != 3:
+ raise ValueError("bank_code_abm must be exactly 3 digits")
+ if len(bank_code_banxico) != 5:
+ raise ValueError("bank_code_banxico must be exactly 5 digits")
if not bank_code_abm.isdigit():
raise NotDigitError
if not bank_code_banxico.isdigit():
raise NotDigitError
if not bank_name.strip():
raise ValueError("bank_name cannot be empty")
- if bank_code_abm in BANKS:
- raise BankCodeABMAlreadyExistsError
- if bank_code_banxico in BANK_NAMES:
- raise BankCodeBanxicoAlreadyExistsError
- BANKS[bank_code_abm] = bank_code_banxico
- BANK_NAMES[bank_code_banxico] = bank_name.strip()
+ # Thread-safe operations
+ with _bank_lock:
+ if bank_code_abm in BANKS:
+ raise BankCodeABMAlreadyExistsError
+ if bank_code_banxico in BANK_NAMES:
+ raise BankCodeBanxicoAlreadyExistsError
+ # Atomic updates
+ BANKS[bank_code_abm] = bank_code_banxico
+ BANK_NAMES[bank_code_banxico] = bank_name.strip()Likely invalid or redundant comment.
tests/test_clabe.py
Outdated
| @@ -36,3 +41,31 @@ def test_generate_new_clabes(): | |||
| for clabe in clabes: | |||
| assert clabe.startswith(prefix) | |||
| assert validate_clabe(clabe) | |||
|
|
|||
|
|
|||
| def test_configure_additional_bank_success(): | |||
There was a problem hiding this comment.
puedes agregar estos bank_codes al test https://github.com/cuenca-mx/spei-validations/blob/5fb22cfd7a590ea573c2c5aa3221206b29768a8c/spei_validations/__init__.py#L6
son los de beta
…th Pydantic model validation
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clabe/validations.py(2 hunks)tests/test_clabe.py(2 hunks)
🔇 Additional comments (4)
tests/test_clabe.py (3)
5-5: LGTM!
The import statement for configure_additional_bank is correctly added.
50-52: Add test cleanup to prevent side effects.
The test modifies global state (BANKS and BANK_NAMES) but doesn't clean up after itself.
55-68: LGTM! Comprehensive error cases.
The test cases cover various invalid input scenarios:
- Invalid format for both codes
- Mixed valid/invalid codes
- Empty bank name
- Non-numeric codes
clabe/validations.py (1)
4-5: LGTM! Appropriate imports.
The pydantic imports are correctly added for the validation requirements.
…k codes - Replaced bank_code_abm with a derived property from bank_code_banxico. - Enhanced validation for bank_code_banxico to ensure it is exactly 5 digits. - Updated tests to reflect changes in bank configuration logic and validation rules.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clabe/validations.py (1)
93-100:⚠️ Potential issueAdd thread safety and improve documentation.
The function modifies global state without thread safety and lacks proper documentation.
Apply these improvements:
+import threading + +_bank_lock = threading.Lock() + def configure_additional_bank(bank_code_banxico: str, bank_name: str) -> None: + """ + Configure an additional bank by updating the BANKS and BANK_NAMES dictionaries. + + Args: + bank_code_banxico: A 5-digit Banxico bank code + bank_name: The name of the bank + + Raises: + ValueError: If validation fails or bank codes already exist + """ request = BankConfigRequest( bank_code_banxico=bank_code_banxico, bank_name=bank_name, ) - BANKS[request.bank_code_abm] = request.bank_code_banxico - BANK_NAMES[request.bank_code_banxico] = request.bank_name + with _bank_lock: + if request.bank_code_abm in BANKS: + raise ValueError(f"ABM code {request.bank_code_abm} already exists") + if request.bank_code_banxico in BANK_NAMES: + raise ValueError(f"Banxico code {request.bank_code_banxico} already exists") + BANKS[request.bank_code_abm] = request.bank_code_banxico + BANK_NAMES[request.bank_code_banxico] = request.bank_name
🧹 Nitpick comments (1)
clabe/validations.py (1)
93-100: Consider adding a remove_bank function.To complete the bank configuration API, consider adding a function to remove banks from the dictionaries. This would be useful for testing and maintenance purposes.
Here's a suggested implementation:
def remove_bank(bank_code_banxico: str) -> None: """ Remove a bank from the BANKS and BANK_NAMES dictionaries. Args: bank_code_banxico: The 5-digit Banxico code of the bank to remove Raises: ValueError: If the bank code doesn't exist """ with _bank_lock: abm_code = bank_code_banxico[-3:] if abm_code not in BANKS or BANKS[abm_code] != bank_code_banxico: raise ValueError(f"Bank with Banxico code {bank_code_banxico} not found") del BANKS[abm_code] del BANK_NAMES[bank_code_banxico]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clabe/validations.py(2 hunks)tests/test_clabe.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_clabe.py
🔇 Additional comments (1)
clabe/validations.py (1)
2-2: LGTM: Imports are properly organized.
The new imports are necessary for the added functionality and follow Python's import organization best practices.
Also applies to: 5-5
clabe/validations.py
Outdated
| class BankConfigRequest(BaseModel): | ||
| bank_name: str = Field( | ||
| min_length=1, | ||
| strip_whitespace=True, | ||
| description="The name of the bank - cannot be empty", | ||
| ) | ||
|
|
||
| bank_code_banxico: str = Field( | ||
| min_length=5, max_length=5, description="The Banxico code for the bank" | ||
| ) | ||
|
|
||
| @validator("bank_code_banxico") | ||
| def validate_bank_code(cls, value): | ||
| if not re.fullmatch(r"\d{5}", value): | ||
| raise ValueError( | ||
| "bank_code_banxico must be a string of exactly 5 digits" | ||
| ) | ||
| return value | ||
|
|
||
| @property | ||
| def bank_code_abm(self): | ||
| return self.bank_code_banxico[-3:] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add class documentation and strengthen validation.
The class needs documentation explaining its purpose and the relationship between Banxico and ABM codes. Additionally, the bank_code_abm property makes assumptions about code derivation.
Add docstring and improve validation:
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
+ - ABM codes (derived from Banxico codes) are valid 3-digit codes
+ """
bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="The name of the bank - cannot be empty",
)
bank_code_banxico: str = Field(
min_length=5, max_length=5, description="The Banxico code for the bank"
)
@validator("bank_code_banxico")
def validate_bank_code(cls, value):
if not re.fullmatch(r"\d{5}", value):
raise ValueError(
"bank_code_banxico must be a string of exactly 5 digits"
)
+ # Validate that the last 3 digits form a valid ABM code
+ abm_code = value[-3:]
+ if abm_code == "000":
+ raise ValueError("Invalid ABM code derived from bank_code_banxico")
return valueCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_clabe.py (2)
56-67: Add more test cases for invalid inputs.The test should cover more edge cases including:
- Duplicate bank codes
- Special characters in bank names
Add these test cases:
@pytest.mark.parametrize( 'banxico_code, name', [ ('1234', 'Test Bank'), # invalid Banxico code 4 digits ('123456', 'Test Bank'), # invalid Banxico code 6 digits ('12345', ''), # Valid code, empty name ('123AT', 'Test Bank'), # Non-numeric codes + ('90713', 'Duplicate Bank'), # Existing Banxico code + ('12345', 'Test Bank\n'), # Name with special characters ], )
70-73: Enhance remove_bank test coverage.The test should verify both dictionaries are cleaned up and handle non-existent bank removal.
Consider this improved implementation:
def test_remove_bank(): + # Setup + original_bank_count = len(BANKS) remove_bank('40138') + + # Verify dictionaries are cleaned + assert len(BANKS) == original_bank_count - 1 + assert '40138' not in BANK_NAMES + + # Verify get_bank_name fails with pytest.raises(ValueError): get_bank_name('138') + + # Verify removing non-existent bank is handled gracefully + remove_bank('99999') # Should not raise any exception
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clabe/__init__.py(1 hunks)clabe/validations.py(2 hunks)tests/test_clabe.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clabe/init.py
🔇 Additional comments (4)
tests/test_clabe.py (2)
5-8: LGTM: Imports are properly organized.
43-53: 🛠️ Refactor suggestion
Enhance test coverage for bank configuration.
The test should verify both the BANKS and BANK_NAMES dictionaries are updated correctly. Also, add cleanup to prevent test interference.
Consider this improved implementation:
+@pytest.fixture(autouse=True)
+def cleanup_banks():
+ original_banks = BANKS.copy()
+ original_bank_names = BANK_NAMES.copy()
+ yield
+ BANKS.clear()
+ BANKS.update(original_banks)
+ BANK_NAMES.clear()
+ BANK_NAMES.update(original_bank_names)
def test_configure_additional_bank_success(abm_code, banxico_code, name):
configure_additional_bank(banxico_code, name)
assert get_bank_name(abm_code) == name
+ assert BANKS[abm_code] == banxico_code
+ assert BANK_NAMES[banxico_code] == nameLikely invalid or redundant comment.
clabe/validations.py (2)
69-91: 🛠️ Refactor suggestion
Add documentation and strengthen validation.
The class needs documentation and additional validation in the bank_code_abm property.
class BankConfigRequest(BaseModel):
+ """
+ Validates and processes bank configuration requests.
+
+ Attributes:
+ bank_name: The name of the bank (non-empty string)
+ bank_code_banxico: 5-digit Banxico bank code
+ bank_code_abm: 3-digit ABM code derived from bank_code_banxico
+ """
bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="The name of the bank - cannot be empty",
)
bank_code_banxico: str = Field(
min_length=5, max_length=5, description="The Banxico code for the bank"
)
@validator("bank_code_banxico")
def validate_bank_code(cls, value):
if not re.fullmatch(r"\d{5}", value):
raise ValueError(
"bank_code_banxico must be a string of exactly 5 digits"
)
return value
@property
def bank_code_abm(self):
+ abm_code = self.bank_code_banxico[-3:]
+ if abm_code == '000':
+ raise ValueError("Invalid ABM code derived from bank_code_banxico")
- return self.bank_code_banxico[-3:]
+ return abm_codeLikely invalid or redundant comment.
93-100:
Add thread safety and duplicate checks.
The function modifies global state without proper synchronization and duplicate checking.
+import threading
+
+_bank_lock = threading.Lock()
+
def configure_additional_bank(bank_code_banxico: str, bank_name: str) -> None:
+ """
+ Configure an additional bank with the given Banxico code and name.
+
+ Args:
+ bank_code_banxico: 5-digit Banxico bank code
+ bank_name: Name of the bank
+
+ Raises:
+ ValueError: If the bank code already exists
+ """
request = BankConfigRequest(
bank_code_banxico=bank_code_banxico,
bank_name=bank_name,
)
- BANKS[request.bank_code_abm] = request.bank_code_banxico
- BANK_NAMES[request.bank_code_banxico] = request.bank_name
+
+ with _bank_lock:
+ if request.bank_code_abm in BANKS:
+ raise ValueError(f"Bank with ABM code {request.bank_code_abm} already exists")
+ if request.bank_code_banxico in BANK_NAMES:
+ raise ValueError(f"Bank with Banxico code {request.bank_code_banxico} already exists")
+
+ BANKS[request.bank_code_abm] = request.bank_code_banxico
+ BANK_NAMES[request.bank_code_banxico] = request.bank_nameLikely invalid or redundant comment.
clabe/validations.py
Outdated
| def remove_bank(bank_code_banxico: str) -> None: | ||
| bank_code_abm = next( | ||
| ( | ||
| abm | ||
| for abm, banxico in BANKS.items() | ||
| if banxico == bank_code_banxico | ||
| ), | ||
| None, | ||
| ) | ||
|
|
||
| if bank_code_abm: | ||
| BANKS.pop(bank_code_abm) | ||
| BANK_NAMES.pop(bank_code_banxico) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve remove_bank implementation.
The function needs thread safety, documentation, and could be simplified.
def remove_bank(bank_code_banxico: str) -> None:
+ """
+ Remove a bank configuration by its Banxico code.
+
+ Args:
+ bank_code_banxico: 5-digit Banxico bank code
+ """
- bank_code_abm = next(
- (
- abm
- for abm, banxico in BANKS.items()
- if banxico == bank_code_banxico
- ),
- None,
- )
+ with _bank_lock:
+ bank_code_abm = next(
+ (abm for abm, banxico in BANKS.items() if banxico == bank_code_banxico),
+ None,
+ )
- if bank_code_abm:
- BANKS.pop(bank_code_abm)
- BANK_NAMES.pop(bank_code_banxico)
+ if bank_code_abm:
+ BANKS.pop(bank_code_abm)
+ BANK_NAMES.pop(bank_code_banxico)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def remove_bank(bank_code_banxico: str) -> None: | |
| bank_code_abm = next( | |
| ( | |
| abm | |
| for abm, banxico in BANKS.items() | |
| if banxico == bank_code_banxico | |
| ), | |
| None, | |
| ) | |
| if bank_code_abm: | |
| BANKS.pop(bank_code_abm) | |
| BANK_NAMES.pop(bank_code_banxico) | |
| def remove_bank(bank_code_banxico: str) -> None: | |
| """ | |
| Remove a bank configuration by its Banxico code. | |
| Args: | |
| bank_code_banxico: 5-digit Banxico bank code | |
| """ | |
| with _bank_lock: | |
| bank_code_abm = next( | |
| (abm for abm, banxico in BANKS.items() if banxico == bank_code_banxico), | |
| None, | |
| ) | |
| if bank_code_abm: | |
| BANKS.pop(bank_code_abm) | |
| BANK_NAMES.pop(bank_code_banxico) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
65-70: 🛠️ Refactor suggestionExpand the documentation with integration details
The documentation needs more context about when and why to use this functionality.
Consider expanding this section to address the previous reviewer's concerns:
-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: +Sin embargo, hemos añadido una función para agregar bancos adicionales a la lista. Esta funcionalidad es útil cuando: + +- Necesitas integrar un banco nuevo que aún no está en la biblioteca +- Requieres hacer pruebas con bancos simulados en ambientes de desarrollo +- Necesitas una solución temporal mientras se actualiza la biblioteca oficialmente + +Esta configuración se puede realizar en tiempo de ejecución, sin necesidad de crear un PR: ```python import clabe clabe.configure_additional_bank('12345', 'New Bank')
+Nota: Esta configuración es temporal y se reinicia cuando se reinicia la aplicación. Para una solución permanente, considere contribuir al proyecto mediante un PR.
</blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>README.md (2)</summary><blockquote> `63-64`: **Enhance the Pydantic v2 migration notice** The migration notice should be more prominent and provide additional context about the impact on existing users. Consider expanding this section with: ```diff -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. +⚠️ **Aviso importante de migración** + +A partir de la versión 2.0.0, el paquete se actualizará a **Pydantic v2**. Esto significa que: +- Las versiones anteriores ya no recibirán soporte +- Los usuarios de Pydantic v1 deberán actualizar sus dependencias +- Se recomienda planificar la migración con anticipación
72-77: Add validation examples and error handlingThe documentation for bank removal should include information about validation and error cases.
Consider expanding this section:
Para eliminar un banco ```python import clabe clabe.remove_bank('12345')
+Importante:
+- La función lanzará un error si el banco no existe
+- No se puede eliminar un banco predefinido de la biblioteca
+- La eliminación es temporal y se reinicia al reiniciar la aplicación</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d84ee8a66e36763be1dea16072ce95798bd1d2bd and acb464465c3dde013a81e649894171d264bc4427. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (1 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…idation logic - Refactored the add_bank function to improve clarity and validation. - Updated tests to align with the new function name and validation rules.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_clabe.py (1)
58-69: Add test cases for duplicate codes and special characters.The test should cover more edge cases:
@pytest.mark.parametrize( 'banxico_code, name', [ ('1234', 'Test Bank'), # invalid Banxico code 4 digits ('123456', 'Test Bank'), # invalid Banxico code 6 digits ('12345', ''), # Valid code, empty name ('123AT', 'Test Bank'), # Non-numeric codes + ('90713', 'Duplicate Bank'), # Duplicate Banxico code + ('12345', 'Test Bank!@#$%'), # Special characters in name ], )README.md (1)
61-77: Enhance documentation with thread safety and error handling.The documentation should include:
- Thread safety implications when modifying banks in a multi-threaded environment
- Examples of error handling for duplicate codes and invalid inputs
Add these examples to the documentation:
Para eliminar un banco ```python import clabe clabe.remove_bank('12345') + +# Error handling examples +try: + clabe.configure_additional_bank('12345', 'Duplicate Bank') # Raises ValueError if bank exists +except ValueError as e: + print(f"Error: {e}") + +# Thread safety note +# Note: Bank configuration operations are thread-safe and can be used in multi-threaded environments</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between acb464465c3dde013a81e649894171d264bc4427 and c94a35abd35ab20baae19f531fa5953449a3474b. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `README.md` (1 hunks) * `clabe/__init__.py` (1 hunks) * `clabe/validations.py` (2 hunks) * `tests/test_clabe.py` (2 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * clabe/__init__.py </details> <details> <summary>🔇 Additional comments (4)</summary> <details> <summary>clabe/validations.py (1)</summary> `92-105`: _:warning: Potential issue_ **Add thread safety and existence validation.** The function needs thread safety when modifying global state and should validate bank code existence: ```diff +import threading + +_bank_lock = threading.Lock() + def add_bank(bank_code_banxico: str, bank_name: str) -> None: """ Add a bank configuration. Args: bank_code_banxico: 5-digit Banxico bank code bank_name: Bank name + + Raises: + ValueError: If the bank code already exists """ request = BankConfigRequest( bank_code_banxico=bank_code_banxico, bank_name=bank_name, ) + request.validate_abm_code() + + with _bank_lock: + if request.bank_code_banxico in BANK_NAMES: + raise ValueError(f"Banxico code {request.bank_code_banxico} already exists") + BANKS[request.bank_code_abm] = request.bank_code_banxico + BANK_NAMES[request.bank_code_banxico] = request.bank_nameLikely invalid or redundant comment.
tests/test_clabe.py (3)
72-83: LGTM! Test covers all necessary scenarios.The test properly verifies bank removal from both dictionaries.
86-92: LGTM! Test handles edge case properly.The test correctly verifies that removing a non-existent bank is handled gracefully.
45-55: 🛠️ Refactor suggestionAdd test cleanup and verify both dictionaries.
The test needs cleanup to prevent side effects and should verify both dictionaries:
+@pytest.fixture(autouse=True) +def cleanup_banks(): + original_banks = BANKS.copy() + original_bank_names = BANK_NAMES.copy() + yield + BANKS.clear() + BANKS.update(original_banks) + BANK_NAMES.clear() + BANK_NAMES.update(original_bank_names) + @pytest.mark.parametrize( 'abm_code, banxico_code, name', [ ('713', '90713', 'Cuenca DMZ'), ('714', '90714', 'Cuenca Gem DMZ'), ('715', '90715', 'Cuenca Gem Beta'), ], ) def test_add_bank_success(abm_code, banxico_code, name): add_bank(banxico_code, name) assert get_bank_name(abm_code) == name + assert BANKS[abm_code] == banxico_code + assert BANK_NAMES[banxico_code] == nameLikely invalid or redundant comment.
| def remove_bank(bank_code_banxico: str) -> None: | ||
| """ | ||
| Remove a bank configuration by its Banxico code. | ||
|
|
||
| Args: | ||
| bank_code_banxico: 5-digit Banxico bank code | ||
| """ | ||
| BANKS.pop(bank_code_banxico[-3:], None) | ||
| BANK_NAMES.pop(bank_code_banxico, None) |
There was a problem hiding this comment.
Add thread safety for bank removal.
The function needs thread safety when modifying global state:
def remove_bank(bank_code_banxico: str) -> None:
"""
Remove a bank configuration by its Banxico code.
Args:
bank_code_banxico: 5-digit Banxico bank code
"""
+ with _bank_lock:
BANKS.pop(bank_code_banxico[-3:], None)
BANK_NAMES.pop(bank_code_banxico, None)Committable suggestion skipped: line range outside the PR's diff.
| 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( | ||
| min_length=1, | ||
| strip_whitespace=True, | ||
| description="Bank name must have at least 1 character.", | ||
| ) | ||
|
|
||
| bank_code_banxico: str = Field( | ||
| regex=r"^\d{5}$", description="Banxico code must be a 5-digit string." | ||
| ) | ||
|
|
||
| @property | ||
| def bank_code_abm(self): | ||
| return self.bank_code_banxico[-3:] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add thread safety and validation for derived ABM code.
The class handles basic validation well using Pydantic, but there are a few improvements needed:
- Add validation that the derived ABM code doesn't conflict with existing codes
- Make the ABM code derivation rules explicit in the docstring
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
+ - ABM codes (derived as last 3 digits of Banxico code) don't conflict
"""
bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="Bank name must have at least 1 character.",
)
bank_code_banxico: str = Field(
regex=r"^\d{5}$", description="Banxico code must be a 5-digit string."
)
@property
def bank_code_abm(self) -> str:
+ """Derive the 3-digit ABM code from the last 3 digits of the Banxico code."""
return self.bank_code_banxico[-3:]
+
+ def validate_abm_code(self) -> None:
+ """Validate that the derived ABM code doesn't already exist."""
+ if self.bank_code_abm in BANKS:
+ raise ValueError(f"ABM code {self.bank_code_abm} already exists")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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( | |
| min_length=1, | |
| strip_whitespace=True, | |
| description="Bank name must have at least 1 character.", | |
| ) | |
| bank_code_banxico: str = Field( | |
| regex=r"^\d{5}$", description="Banxico code must be a 5-digit string." | |
| ) | |
| @property | |
| def bank_code_abm(self): | |
| return self.bank_code_banxico[-3:] | |
| 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 | |
| - ABM codes (derived as last 3 digits of Banxico code) don't conflict | |
| """ | |
| bank_name: str = Field( | |
| min_length=1, | |
| strip_whitespace=True, | |
| description="Bank name must have at least 1 character.", | |
| ) | |
| bank_code_banxico: str = Field( | |
| regex=r"^\d{5}$", description="Banxico code must be a 5-digit string." | |
| ) | |
| @property | |
| def bank_code_abm(self) -> str: | |
| """Derive the 3-digit ABM code from the last 3 digits of the Banxico code.""" | |
| return self.bank_code_banxico[-3:] | |
| def validate_abm_code(self) -> None: | |
| """Validate that the derived ABM code doesn't already exist.""" | |
| if self.bank_code_abm in BANKS: | |
| raise ValueError(f"ABM code {self.bank_code_abm} already exists") |
…ditional_bank to add_bank
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
63-65: 🛠️ Refactor suggestionEnhance the Pydantic v2 upgrade notice
The notice about Pydantic v2 upgrade needs more context. Consider adding:
- Timeline for the v2.0.0 release
- Migration guide or link to one
- Impact on existing code using Pydantic v1
🧹 Nitpick comments (1)
README.md (1)
60-77: Add integration example with a real projectTo address the previous review comment and provide more context, consider adding a complete example that demonstrates a real-world use case:
+## Ejemplo de integración + +Este ejemplo muestra cómo utilizar las funciones de configuración de bancos en un proyecto real: + +```python +from clabe import add_bank, remove_bank, validate_clabe + +def setup_temporary_bank(): + # Configurar un banco temporal para procesamiento + try: + add_bank('12345', 'Banco Temporal') + + # Validar CLABE con el nuevo banco + clabe_number = '123450000000000008' + is_valid = validate_clabe(clabe_number) + + # Procesar transacciones... + + finally: + # Limpiar configuración temporal + remove_bank('12345') +``` + +Este patrón es útil cuando: +- Se necesita procesar CLABE de bancos en proceso de integración +- Se realizan pruebas con bancos simulados +- Se manejan bancos legacy que no están en la lista principal
| 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') | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand the add_bank documentation
The documentation should provide more details about:
- The persistence of added banks (are they temporary or permanent?)
- Valid format requirements for bank_code and bank_name
- Error handling scenarios
- Use cases for when this functionality would be needed
Example:
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')+### Consideraciones importantes
+
+- Los bancos agregados son temporales y se reiniciarán cuando se reinicie la aplicación
+- El código del banco debe ser un string de 5 dígitos
+- El nombre del banco no debe estar vacío
+
+### Manejo de errores
+
+```python
+try:
- clabe.add_bank('12345', 'New Bank')
+except ValueError as e: - print(f"Error al agregar banco: {e}")
+```
<!-- This is an auto-generated comment by CodeRabbit -->
| Para eliminar un banco | ||
|
|
||
| ```python | ||
| import clabe | ||
| clabe.remove_bank('12345') | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document remove_bank behavior and validation
The remove_bank documentation should clarify:
- What happens when trying to remove a non-existent bank
- Whether removing a bank affects existing CLABE numbers
- Error handling scenarios
Example:
Para eliminar un banco
```python
import clabe
clabe.remove_bank('12345')+### Consideraciones importantes
+
+- No se pueden eliminar bancos predeterminados del sistema
+- Intentar eliminar un banco inexistente generará un error
+
+### Manejo de errores
+
+```python
+try:
- clabe.remove_bank('12345')
+except ValueError as e: - print(f"Error al eliminar banco: {e}")
+```
<!-- This is an auto-generated comment by CodeRabbit -->
#169) * ci: add Codecov token to workflow configuration * feat: add error handling for existing bank codes and names * ci: update GitHub Actions workflow to use new action versions * chore: bump version to 1.2.17.dev1 * chore: update GitHub Actions workflows for release and testing * fix: update PyPI token reference in GitHub Actions workflow * fix: update PyPI token reference in release workflow to use PYPI_API_TOKEN * refactor: rename and update error classes for bank code validation * feat: add configure_additional_bank function and corresponding tests * docs: update README to include instructions for add a new bank * chore: bump version to 1.2.17.dev2 * chore: bump version to 1.2.17 * chore: update GitHub Actions workflows for release and testing * fix: update PyPI token reference in GitHub Actions workflow * fix: update PyPI token reference in release workflow to use PYPI_API_TOKEN * refactor: rename and update error classes for bank code validation * feat: add configure_additional_bank function and corresponding tests * docs: update README to include instructions for add a new bank * chore: bump version to 1.2.17.dev2 * chore: bump version to 1.2.17 * feat: enhance configure_additional_bank with input validation * refactor: streamline bank configuration by replacing error classes with Pydantic model validation * refactor: update BankConfigRequest model and validation logic for bank codes - Replaced bank_code_abm with a derived property from bank_code_banxico. - Enhanced validation for bank_code_banxico to ensure it is exactly 5 digits. - Updated tests to reflect changes in bank configuration logic and validation rules. * feat: add remove_bank function to manage bank entries * docs: update README to include instructions for adding a new bank * refactor: rename configure_additional_bank to add_bank and update validation logic - Refactored the add_bank function to improve clarity and validation. - Updated tests to align with the new function name and validation rules. * docs: update README to reflect function name change from configure_additional_bank to add_bank --------- Co-authored-by: gabino <[email protected]>
Description
This pull request introduces the following changes:
New Functionality
add_bankfunction to configure an additional bank by updating theBANKSandBANK_NAMESdictionaries.remove_bankfunction to delete a bank by updating theBANKSandBANK_NAMESdictionaries.Version Update
1.2.16to1.2.17to reflect the addition of the new functionality.Unit Tests
BANKSandBANK_NAMESwhen provided valid inputs.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores