feat: oxidized progress bars#426
Conversation
The `Message`/`MessageType` implementation would've caused there to be fields on `Message` that only applied to some `MessageType`s. For example, the upcoming progress bar `MessageType` would have needed a `total` field on `Message`, but `MessageType::Text` has no use for such a field.
| pub(crate) model: MessageType, | ||
| /// Types of message for printing. | ||
| #[derive(Clone, Debug)] | ||
| pub enum Event { |
There was a problem hiding this comment.
I found this refactor was necessary because the previous Message/MessageType model meant that all messages would require the same set of fields in a Message instance, even if their associated MessageType did not care about the fields. For example, a hypothetical MessageType::FinishProgressBar would never care about the Message.permanent field.
| _ = progress_bar.insert(npb.bar); | ||
| } | ||
| Event::UpdateProgressBar(delta) => match progress_bar { | ||
| None => unreachable!(), |
There was a problem hiding this comment.
All of these unreachable!() blocks are such because the condition is already checked on the Printer. The reason it isn't checked here is that the InnerPrinter's thread ("printer thread" from here on) can panic and have its fail state not be noticed until the end of execution. From the main thread's side, this is only noticed at the end of execution when it tries to join the printer thread's handle again and discovers an error. Therefore, if we want an immediate error upon improper use, it has to originate from the main thread.
I'm not a huge fan of this architecture and would be happy to revisit it, but any alternative would've been well out of scope for this PR.
d807df8 to
99dfb44
Compare
| should_timestamp: bool, | ||
| } | ||
|
|
||
| #[bon] |
There was a problem hiding this comment.
This macro allows the easy creation of builder-style object instantiation. In this particular use case, it converts the new method below into a method on Progresser called build, where each parameter of the method is instead a builder method.
This is a really common pattern in Rust that developers of this crate will expect, but the Python wrapper converts this builder pattern into a kwargs-based pattern for familiarity on the Python side.
| #[builder(default)] show_progress: bool, | ||
| #[builder(default)] show_percentage: bool, | ||
| ) -> PyResult<Self> { | ||
| let mut template_str = String::from("{msg} [{wide_bar}]"); |
There was a problem hiding this comment.
See https://docs.rs/indicatif/0.18.3/indicatif/index.html#templates for an explanation of the keys used throughout this method
mr-cal
left a comment
There was a problem hiding this comment.
Looks nice overall!
Here's a few things:
If I raise an error midway through, it raises the error then prints a completed progress bar. I'd expect the progress bar to (1) not pretend it completed and (2) not print after an exception.
I'm also noticing the printing isn't deterministic. Look where boop is in these 3 consecutive runs:
I can only get it to print out of order in like 1 of 20 runs.
repro:
import logging
from time import sleep
from craft_cli import Emitter, Verbosity
logger = logging.getLogger(__name__)
logger.setLevel(logging.DEBUG)
e = Emitter(Verbosity.TRACE, "blah.log", "Hello", streaming_brief=True)
logger.info("Testing")
TOTAL_NUMBERS = 999
with e.progress_bar(
"Crunching the numbers", TOTAL_NUMBERS, show_progress=True, show_percentage=True
) as prog:
# An overflow just keeps counting over the total.
for i in range(TOTAL_NUMBERS + 10):
prog.tick()
sleep(0.1)
if i % 25 == 0:
prog.inc(20)
logger.warning("⚠️ Booping imminent ⚠️")
sleep(0.1)
prog.println("Boop")
raise ValueError("boom")
e.message("Done")And a last comment, should the println messages go to the log?
I fixed (1), but (2) and the nondeterministic printing I think are better handled in/after CRAFT-4985 (adding CraftError handling). I'll have to create a standardized way to raise errors and make sure the printing thread stops yapping before surfacing the traceback. I'll take this as a win though. It ain't true multi-threaded programming unless there's nondeterminism. |
There was a problem hiding this comment.
Pull request overview
Adds a new Rust-backed progress bar API to the Craft CLI emitter/printer pipeline, refactoring printer messages into a richer Event model to support progress bar lifecycle + printing while a bar is active.
Changes:
- Replaced the old
Message/MessageTypeprinter payload withEvent+Text(and added progress-bar events). - Implemented a new PyO3
Progressercontext manager (Emitter.progress_bar(...)) backed byindicatif. - Updated stream/log listeners to emit
Event::Stream/Event::Logand addedbonas a dependency.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/printer.rs | Introduces Event model, progress-bar lifecycle handling, and routing/validation logic in Printer::send. |
| src/progress.rs | New PyO3 Progresser context manager and indicatif progress bar construction/printing APIs. |
| src/emitter.rs | Exposes Emitter.progress_bar(...) returning Progresser; updates emits to use Event::Text(Text). |
| src/streams.rs | Emits Event::Stream(Text) instead of the old Message struct. |
| src/logs.rs | Emits Event::Log(Text) instead of the old Message struct. |
| src/lib.rs | Registers the new Rust module (mod progress). |
| craft_cli/_rs/progress.pyi | Adds typing for the new Progresser API. |
| craft_cli/_rs/emitter.pyi | Adds typing for Emitter.progress_bar(...) -> Progresser. |
| Cargo.toml / Cargo.lock | Adds the bon dependency and locks transitive deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
debcbcf to
b55132c
Compare
make lint && make test?Implements the
progress_barmethod for the emitter. This one has an entirely different API from the existing one, but comes with far more content.The exact appearance of the progress bar doesn't need much review, as the design team and I have not landed on an approved appearance yet.
I strongly recommend reviewing per-commit.
Here's a script that showcases some of the new behavior and gives a good starting point for testing by hand:
CRAFT-4983