-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display elapsed time in datagen log #4469
Conversation
provider/datagen/src/driver.rs
Outdated
.into_par_iter() | ||
.map(|locale| { | ||
let instant2 = Instant::now(); | ||
load_with_fallback(key, &locale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do double-check that the old and new behaviors are the same. I switched things around to avoid duplicating the duration logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feature lgtm, small issue around code readability
provider/datagen/src/driver.rs
Outdated
let instant2 = Instant::now(); | ||
load_with_fallback(key, &locale) | ||
.transpose()? | ||
.map(|payload| sink.put_payload(key, &locale, &payload)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm not convinced the functional style is clearer here, especially with the timing code, where you kinda actually do need to know where it should run. I think we should revert to the older imperative style here and just add timing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree but in this case I was confused why we had an expression like
if let Some(payload) = load_with_fallback(key, &locale) {
sink.put_payload(key, &locale, &payload?)
} else {
Ok(())
}
// followed by other chaining logic
Also note that the majority of the diff in this branch is due to changing try_for_each
to map
so that I can call .max()
on the result.
I will do another round of making this more clear though. I realized that I might be losing the error context with the new .transpose()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a bit better. still not convinced that having the .elapsed()
in the highly functional code is a good idea
Is this changelog worthy? |
shrug when I make a changelog manually I would include this. Otherwise ... eh |
Changelog in #4484 |
Makes the output more useful/actionable.
cargo make testdata: