Move to a type-oriented design pattern#8
Conversation
|
@mentonin Isso está pronto, ou ainda é um draft? |
There was a problem hiding this comment.
Pull request overview
This PR prototypes a type-oriented API for CPF by introducing a Cpf type that enforces validation at construction/parsing, and updates crate exports/tests to use the new type-centric interface.
Changes:
- Make
cpfa private module and re-exportCpffrom the crate root. - Replace string-based CPF helpers with a validated
Cpfnewtype +FromStr/Displayimplementations. - Rewrite CPF unit tests to validate parsing, error cases, generation, and formatting via
Display.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/lib.rs | Makes cpf private and re-exports Cpf; updates integration-style tests to use Cpf::from_str/to_string. |
| src/cpf.rs | Replaces the old function-based CPF API with a Cpf type, parsing/validation logic, generation, and updated tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| && input.chars().nth(10).unwrap().to_digit(10).unwrap() == digit2 as u32 | ||
| impl Display for Cpf { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| let digits: String = self.0.iter().map(|d| (d + b'0') as char).collect(); |
There was a problem hiding this comment.
Display::fmt builds digits with self.0.iter().map(|d| (d + b'0') as char), but d is a &u8, so d + b'0' won’t compile. Dereference/copy the byte before adding (or use a formatting approach that avoids manual ASCII arithmetic).
| let digits: String = self.0.iter().map(|d| (d + b'0') as char).collect(); | |
| let digits: String = self.0.iter().copied().map(|d| (d + b'0') as char).collect(); |
| WrongLength, | ||
| NonNumeric, | ||
| WrongChecksum, | ||
| BlackListed, |
There was a problem hiding this comment.
ParseCpfError::BlackListed doesn’t follow Rust’s usual CamelCase spelling for a single word (“Blacklisted”). Renaming to Blacklisted would make the public API more idiomatic and consistent with the other variants.
| BlackListed, | |
| Blacklisted, |
| } | ||
| let mut digits = [0; Self::SIZE]; | ||
| for (c, d) in s.chars().zip(digits.iter_mut()) { | ||
| *d = c.to_digit(10).ok_or(ParseCpfError::NonNumeric)? as u8; |
There was a problem hiding this comment.
from_str uses c.to_digit(10) to parse digits, which will accept non-ASCII Unicode digits. Other validators in this crate consistently use is_ascii_digit(). If CPF parsing is intended to only accept ASCII digits, consider validating with is_ascii_digit (and converting via to_digit/byte math only after that) to keep behavior consistent across modules.
| *d = c.to_digit(10).ok_or(ParseCpfError::NonNumeric)? as u8; | |
| if !c.is_ascii_digit() { | |
| return Err(ParseCpfError::NonNumeric); | |
| } | |
| *d = c as u8 - b'0'; |
| pub mod renavam; | ||
| pub mod voter_id; | ||
|
|
||
| pub use cpf::Cpf; |
There was a problem hiding this comment.
Cpf implements FromStr with type Err = ParseCpfError, but cpf is now a private module (mod cpf;) and ParseCpfError is not re-exported. This will either fail to compile due to a private type leaking into a public interface, or prevent downstream users from naming/matching on the error. Consider making the module public again (pub mod cpf;), or re-exporting the error type (e.g., pub use cpf::ParseCpfError;), or moving the error type to a publicly reachable path.
| pub use cpf::Cpf; | |
| pub use cpf::{Cpf, ParseCpfError}; |
| let mod_sum = base | ||
| .iter() | ||
| .enumerate() | ||
| .fold(0, |acc, (i, d)| (acc + (10 - i as u8) * d) % 11); |
There was a problem hiding this comment.
In hashdigit, d is a &u8 from .iter(), but it’s used directly in arithmetic (... * d). This won’t compile (needs dereference/copy), and also makes the intent a bit unclear. Adjust the closure to operate on u8 values (e.g., by destructuring |acc, (i, &d)| ... or dereferencing *d).
| .fold(0, |acc, (i, d)| (acc + (10 - i as u8) * d) % 11); | |
| .fold(0, |acc, (i, &d)| (acc + (10 - i as u8) * d) % 11); |
Ainda é um draft, mas a ideia mudaria a estrutura da biblioteca. Eu gostaria de entender se essa mudança é desejada antes de trabalhar mais nisso. |
Por mim tudo bem. Só subir a versão maior no lançamento da versão e documentar as mudanças na API. |
To take advantage of Rust type-safety features, it would be interesting to make CPF, CNPJ, etc. formal Types, with data validation at creation.
This would change most or all of the public interface, but I believe it would be a more robust interface, and not an issue for the crate at this stage.
The bigger problem would be incompatibility with the other brazilian-utils interfaces, though those could maybe also change.
I wrote a quick implementation of how I envision this working for the CPF module, and can implement it for the other data types if this moves forward.