Skip to content

Conversation

@Mathious6
Copy link
Collaborator

Why?

The CLI had tightly coupled parsing, IO, and domain logic in a single flow, making it hard to test & extend.

What?

  • Version: Bump to 1.0.0
  • Dependencies: Refresh tokio, anyhow, directories, serde_json, tracing-*, rpassword and add tracing-appender
  • Architecture
    • New modules: commands/, services/, settings/, ux/
    • Extract AuthService with CredentialsProvider + ClientFactory
    • settings/ split into consts.rs, logging.rs, store.rs (KISS SettingsStore + FileSettingsStore)
    • main.rs reduced to: init logger → parse Clirun(cli)
  • Commands
    • accounts, trade order new, transfer: now use AuthService
    • config: persists client_number via FileSettingsStore
    • quote: typed args (length: i64, interval: i64), structured logging
  • UX: New ux::TextProgressBar (reusable + replaces inline progress)
  • Logging
    • Structured JSON file via tracing-subscriber + tracing-appender
    • Log file under platform data dir (e.g., ~/.local/share/bourso/bourso-cli/bourso.log)
    • Console layer remains compact and respects RUST_LOG

Main impact?

  • Breaking changes
    • config flag renamed: --username--client-number
    • quote args now numeric (length: i64, interval: i64)
  • Paths
    • Settings: platform config dir (e.g., ~/.config/bourso/bourso-cli/settings.json)
    • Logs: platform data dir (e.g., ~/.local/share/bourso/bourso-cli/bourso.log)
  • Maintainability
    • Clear separation of concerns (CLI ↔ services ↔ API)
    • Easier testing/mocking (boxed traits in AuthService)
    • Reusable UX components and centralized logging init

What next?

  • refactor src/bourso_api

Copy link
Owner

@azerpas azerpas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only starting the review but here's a few things already

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried to run the CLI and it seems that these info! logs are not displaying the quotes correctly. 😕
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably broke some logs during development, as handling them will be the subject of a dedicated pull request. In the meantime, I suggest ignoring them (especially the console logs) and waiting for the command-line interface to stabilize before fixing them 🫡

However, I was asked a question: should this type of logging be handled in the command-line interface or in the API? In my opinion, it makes more sense to handle it at the API level, since it's the result of our API call, and the command-line interface doesn't add any information about it. Furthermore, when we run the API on its own, maintaining this type of logging seems really important ?

@Mathious6
Copy link
Collaborator Author

During development, I noticed that I had broken the --credentials functionality; this has already been fixed and will be addressed in a new PR :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants