-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix: correctly pluralize 'target'/'targets' in finished message #16132
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
base: master
Are you sure you want to change the base?
Conversation
| ); | ||
| let target_count = self.counts.len(); | ||
| let message = format!( | ||
| "{profile_link}`{profile_name}` profile [{opt_type}]{profile_link:#} target(s) in {time_elapsed}", |
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.
This message is emitted in every build command, and changing this failed 960 tests. To fix them we then need to update snapshots in those 960 tests. I am not sure if it is worthy.
| let profile_link = build_runner.bcx.gctx.shell().err_hyperlink( | ||
| "https://doc.rust-lang.org/cargo/reference/profiles.html#default-profiles", | ||
| ); | ||
| let target_count = self.counts.len(); |
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.
This seems wrong as it counted every package in the dependency graph, not just selected local packages.
This change ensures that the finished message correctly uses "target" or "targets"
based on the number of targets built, improving clarity in multi-target builds.
However, maybe the question should be asked like: What do we really want to count here? What does target(s) mean here really?
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
This change ensures that the finished message correctly uses "target" or "targets"
based on the number of targets built, improving clarity in multi-target builds.