1.0.0: CLI architecture refactor, typed domain models & new settings/logging stack #56
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…streamline file storage implementation
…ct directory handling
…tings module to use them
…ined logging initialization
…entialsProvider and ClientFactory traits
…tore for file-based settings management
…r handling in commands module
…implified imports
…n in the transfer command
…ter file handling
…improve memory management
…validation in bourso_api
…r improved clarity
…roved type safety
…ds for improved type safety and clarity
…d updating password type to improve type safety
…SettingsStore for improved settings management
…and consolidate settings imports
…tings store and streamline password/MFA handling
…eamline argument handling in commands
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1.0 #56 +/- ##
===========================================
- Coverage 33.55% 22.86% -10.69%
===========================================
Files 9 12 +3
Lines 304 446 +142
===========================================
Hits 102 102
- Misses 202 344 +142 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
azerpas
reviewed
Nov 29, 2025
Owner
azerpas
left a comment
There was a problem hiding this comment.
A few interrogations on the types. Nonetheless, awesome job, thank you Mathis!
| Password, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, AsRef, From, Into)] |
Owner
There was a problem hiding this comment.
Are all these derives required? Cause I feel like more derives, more code, more time to compile, bigger file size
| pub struct MoneyAmount(f64); | ||
| impl MoneyAmount { | ||
| pub fn new(v: f64) -> Result<Self, ValueError> { | ||
| if v > 0.0 && v.fract().abs() <= 0.02 { |
Owner
There was a problem hiding this comment.
Not sure what the logic is here with v.fract().abs() <= 0.02?
Comment on lines
+236
to
+254
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, AsRef, From, Into)] | ||
| #[serde(try_from = "String", into = "String")] | ||
| pub struct Password(String); | ||
| impl Password { | ||
| pub fn new(s: &str) -> Result<Self, ValueError> { | ||
| let t = s.trim(); | ||
| if !t.is_empty() { | ||
| Ok(Self(t.into())) | ||
| } else { | ||
| Err(ValueError::Password) | ||
| } | ||
| } | ||
| } | ||
| impl FromStr for Password { | ||
| type Err = ValueError; | ||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Self::new(s) | ||
| } | ||
| } |
Owner
There was a problem hiding this comment.
Debug on Password isn't a good idea IMO, would risk exposing the password to logs. For consistency, let's still implement Debug but with a redacted string.
Suggested change
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, AsRef, From, Into)] | |
| #[serde(try_from = "String", into = "String")] | |
| pub struct Password(String); | |
| impl Password { | |
| pub fn new(s: &str) -> Result<Self, ValueError> { | |
| let t = s.trim(); | |
| if !t.is_empty() { | |
| Ok(Self(t.into())) | |
| } else { | |
| Err(ValueError::Password) | |
| } | |
| } | |
| } | |
| impl FromStr for Password { | |
| type Err = ValueError; | |
| fn from_str(s: &str) -> Result<Self, Self::Err> { | |
| Self::new(s) | |
| } | |
| } | |
| #[derive(Clone, PartialEq, Eq, Hash, Serialize, Deserialize, AsRef, From, Into)] | |
| #[serde(try_from = "String", into = "String")] | |
| pub struct Password(String); | |
| impl Password { | |
| pub fn new(s: &str) -> Result<Self, ValueError> { | |
| let t = s.trim(); | |
| if !t.is_empty() { | |
| Ok(Self(t.into())) | |
| } else { | |
| Err(ValueError::Password) | |
| } | |
| } | |
| } | |
| impl FromStr for Password { | |
| type Err = ValueError; | |
| fn from_str(s: &str) -> Result<Self, Self::Err> { | |
| Self::new(s) | |
| } | |
| } | |
| impl std::fmt::Debug for Password { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.write_str("Password(***REDACTED***)") | |
| } | |
| } |
Owner
There was a problem hiding this comment.
I've added some tests for this file and I think some our validations are a bit broken:
Tests
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_client_number() {
assert!(ClientNumber::new("12345678").is_ok());
assert!(ClientNumber::new("1234abcd").is_err());
assert!(ClientNumber::new("1234567").is_err());
assert!(ClientNumber::new("123456789").is_err());
}
#[test]
fn test_account_id() {
assert!(AccountId::new("a1b2c3d4e5f60718293a4b5c6d7e8f90").is_ok());
assert!(AccountId::new("g1b2c3d4e5f60718293a4b5c6d7e8f90").is_err());
assert!(AccountId::new("a1b2c3d4e5f60718293a4b5c6d7e8f9").is_err());
assert!(AccountId::new("a1b2c3d4e5f60718293a4b5c6d7e8f900").is_err());
}
#[test]
fn test_symbol_id() {
assert!(SymbolId::new("AMZN").is_ok());
assert!(SymbolId::new("1rPAF").is_ok());
assert!(SymbolId::new("1rPAIR").is_err());
assert!(SymbolId::new("1rTWPEA").is_err());
assert!(SymbolId::new("ABC@123").is_err());
assert!(SymbolId::new("A").is_err());
assert!(SymbolId::new("ABCDEFGHIJKLM").is_err());
}
#[test]
fn test_order_quantity() {
assert!(OrderQuantity::new(10).is_ok());
assert!(OrderQuantity::new(0).is_err());
}
#[test]
fn test_money_amount() {
assert!(MoneyAmount::new(100.0).is_ok());
assert!(MoneyAmount::new(0.0).is_err());
assert!(MoneyAmount::new(-50.0).is_err());
assert!(MoneyAmount::new(25.999).is_err());
assert!(MoneyAmount::new(30.54).is_ok());
}
#[test]
fn test_transfer_reason() {
assert!(TransferReason::new("Salary").is_ok());
assert!(TransferReason::new("Bonus2021").is_err());
assert!(TransferReason::new("ThisReasonIsWayTooLongToBeValidBecauseItExceedsFiftyCharacters").is_err());
}
#[test]
fn test_mfa_code() {
assert!(MfaCode::new("123456").is_ok());
assert!(MfaCode::new("12345").is_err());
assert!(MfaCode::new("123456789012").is_ok());
assert!(MfaCode::new("1234567890123").is_err());
assert!(MfaCode::new("12345A").is_err());
}
#[test]
fn test_password() {
assert!(Password::new("12345678").is_ok());
assert!(Password::new("").is_err());
assert!(Password::new(" ").is_err());
}
}
- MoneyAmounts are only valid with *.01 or *.02 amounts
- Validation for symbols starts at 6 when AMZN for instance is at 4
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, AsRef, From, Into)] | ||
| #[serde(try_from = "String", into = "String")] |
Owner
There was a problem hiding this comment.
I think the try_from logic is a bit buggy. And it goes the same for ClientNumber
Using TryFrom, we'll use auto-generated From which don't use our constructor thus bypass our validation. It can be verified with some tests:
Tests
fn password_deserialization_rejects_empty() { let json = r#""""#; // empty string let result: Result = serde_json::from_str(json); assert!(result.is_err(), "Empty password should be rejected during deserialization"); }We probably need something like
impl TryFrom<String> for Password {
type Error = ValueError;
fn try_from(s: String) -> Result<Self, Self::Error> {
Self::new(&s)
}
}Same goes for ClientNumber
| types::{ClientNumber, MfaCode, Password}, | ||
| }; | ||
|
|
||
| // TODO: does it make sense to have MFA handling in the CLI? |
Owner
There was a problem hiding this comment.
good question, how do you this handled?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why?
What?
Version
1.0.0bourso-cliandsrc/bourso_apias membersDependencies
tokio,anyhow,clap,directories,serde_json,tracing-*,futures-util,rpasswordtracing-appenderon the CLI sidethiserror+derive_moreinbourso_apifor typed domain valuesArchitecture
commands/,services/,settings/,ux/AppCtx { settings_store }andrun(Cli)entrypoint to decouple CLI wiring from command logicCredentialsProvider+ClientFactoryto isolate login/MFA handlingsettings/split into:consts.rs(app identifiers + filenames)logging.rs(centralizedinit_logger()usingProjectDirs+tracing-appender)store.rs(SettingsStoretrait +FsSettingsStoreimplementation): ready for Vault supportmain.rsreduced to: init logger → parseCli(derive) →run(cli)Domain types (
bourso_api::types)bourso_apiwith validated value types:ClientNumber,AccountId,SymbolIdOrderQuantity,MoneyAmount,TransferReasonMfaCode,PasswordQuoteLength,QuotePeriod,OrderSide(moved here fromtrade::order)ValueErrorusingthiserrorfor input/validation errorsFromStr+value_parser!, reducing primitive obsession and centralizing validationCommands
accountstrade order newtransferconfigClientNumberviaFsSettingsStoreClientNumbernewtype instead of rawStringquotesymbol: SymbolIdlength: QuoteLength(1, 5, 30, 90, 180, 365, 1825, 3650)period: QuotePeriod(currently only0is accepted)trade order newaccount: AccountIdside: OrderSide(with default"buy")symbol: SymbolIdquantity: OrderQuantitytransferfrom_account: AccountId,to_account: AccountIdamount: MoneyAmountreason: Option<TransferReason>Cli.credentialsis now aPathBuf(parsed viavalue_parser!(PathBuf)) and feedsFsSettingsStore::from_pathwhen providedUX
ux::TextProgressBarclamp) and handlestotal == 0gracefullyLogging
tracing-subscriber+tracing-appender::rolling::never~/.local/share/bourso/bourso-cli/bourso.log)RUST_LOGMain impact?
Breaking changes
configflag renamed:--username➜--client-numberquote:--intervalrenamed to--period--lengthis now aQuoteLengthenum (must be one of1,5,30,90,180,365,1825,3650)--periodcurrently only accepts0(validated viaQuotePeriod)client_numbermust be 8 digitsaccountIDs must be 32 hex charssymbolIDs must be 6–12 alphanumeric charsquantitymust be a strictly positive integeramountmust be positive with up to 2 decimalsPaths
e.g.
~/.config/bourso/bourso-cli/settings.jsone.g.
~/.local/share/bourso/bourso-cli/bourso.logMaintainability
commands/*(orchestration)services/*(Auth, etc.)bourso_api(HTTP/domain)AuthServicetakes boxedCredentialsProvider+ClientFactorySettingsStoretrait enables in-memory / custom implementationsbourso_api::typesensure CLI and API use the same invariantsWhat next?
src/bourso_apisrc/bourso_api/typesto dedicated domain folder