Support alphanumeric CNPJ identifiers in CSV ingestion - #44
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Get.valid() still contains a large unreachable legacy checksum block after an early return, which must be removed to avoid future maintenance mistakes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the CSV ingestion pipeline to support the new alphanumeric CNPJ format accepted by ReceitaWS, ensuring formatted CSV values can be normalized and validated before being queried.
Changes:
- Updated
Get.format()to strip punctuation while preserving alphanumeric characters and normalizing to uppercase. - Extended
Get.valid()to accept alphanumeric CNPJs (12 alphanumeric + 2 numeric check digits) and validate check digits using a shared_check_digit()routine. - Added regression tests and updated README docs (PT/EN) to describe the supported formats and validation behavior.
File summaries
| File | Description |
|---|---|
| receita/tools/get.py | Implements alphanumeric-aware normalization and validation, introducing _check_digit() and updated format()/valid() logic. |
| tests/test_get.py | Adds regression coverage for formatting, validation, and CSV reading with both numeric and alphanumeric CNPJs. |
| README.rst | Documents acceptance of numeric/alphanumeric CNPJ formats and pre-checking of check digits (PT). |
| README.en.rst | Documents acceptance of numeric/alphanumeric CNPJ formats and pre-checking of check digits (EN). |
Review details
Suppressed comments (1)
receita/tools/get.py:141
- There is unreachable legacy checksum code left after the early return, so lines 139-167 can never execute. This makes the function confusing to maintain and risks future edits happening in the dead path instead of the active one; please delete the leftover block so valid() has a single source of truth.
if self._check_digit(base) != int(digits[0]):
return False
return self._check_digit(base + digits[0]) == int(digits[1])
tam = 12
nums = cnpj[:tam]
digs = cnpj[tam:]
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Took this over and pushed a fix on top, as the checksum was being skipped for the new format. What was happening
if not cnpj.isdigit():
return True
What changedThe premise that the alphanumeric format "cannot be validated with the numeric checksum algorithm" is not the case. The numeric format is a subset of the alphanumeric one: the same modulus 11 rules apply when each character is converted with if not re.fullmatch(r"[0-9A-Z]{12}[0-9]{2}", cnpj):
return FalseThe shape check also enforces numeric check digits, which the previous Verification
Tests were rewritten, as the previous ones asserted the invalid example as valid. They now cover both formats, wrong check digits, non numeric check digits, length and punctuation, the published sample, and that |
There was a problem hiding this comment.
🔵 Needs a closer look
Get.valid() still contains unreachable legacy validation code after the new early return, and it should be removed before merging to avoid maintainability issues.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tests/test_get.py:9
- The _cnpj() test helper currently calls getter._check_digit(), which is part of the implementation under test; this makes the alphanumeric test cases self-fulfilling (they can pass even if _check_digit() is wrong). Prefer computing the digits independently in the test helper (or using fixed known-good samples) so the tests can catch checksum regressions.
receita/tools/get.py:140 - After the new checksum checks, valid() returns on line 137, but the old numeric-only validation logic (starting at line 139) is still present below and is now unreachable dead code. Please delete the legacy block to keep the function maintainable and avoid future divergence between two checksum implementations.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
The CSV input stripped every character that was not a digit and required fourteen digits, so identifiers in the new alphanumeric format were discarded before reaching the webservice. Keep letters when normalizing the input and validate both formats with the same rules: twelve alphanumeric characters followed by two numeric check digits. The numeric format is a subset of the alphanumeric one, so the existing modulus 11 checksum applies to both once each character is converted through the ASCII table, which preserves the value of the digits. Verifying the check digits still gates the request, so an invalid identifier does not consume a query. Behaviour for numeric CNPJs is unchanged, and the published sample for the alphanumeric format is covered by the tests. Closes #38 Co-Authored-By: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7ccf2e7 to
36b8b09
Compare
The Receita API now accepts alphanumeric CNPJ values, but the project was still stripping non-digit characters and rejecting anything that was not a 14-digit numeric identifier. This prevented valid newer CNPJ values from being fetched through the CLI pipeline.
What changed
Example