-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Merged by Bors] - fix diagnostic length for asset count #2165
Conversation
Please correct me if I'm wrong, but the length of the string is still capped at Edit: oh, now I see the additional logic for the asset names! |
/// The width which diagnostic names will be printed as | ||
/// Plugin names should not be longer than this value | ||
pub const MAX_DIAGNOSTIC_NAME_WIDTH: usize = 32; |
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.
What's the purpose of having this limit in the first place?
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.
it came in #1505
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.
it was to clean up how it's displayed, and it's an issue only for this diagnostics as all other have a fixed name
if asset_type_name.len() > max_length { | ||
asset_type_name | ||
.split_at(asset_type_name.len() - max_length + 1) | ||
.1 |
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.
.1 | |
.0 |
This should be the first &str
returned by split_at
(assuming you want the first part of the type name).
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.
I want the second, the first is most likely to be part of the path: asset_count bevy_render::texture::texture::Texture
Looks good to me! |
bors r+ |
fixes #2156 limit the diagnostic name to `MAX_DIAGNOSTIC_NAME_WIDTH` length
Pull request successfully merged into main. Build succeeded: |
fixes bevyengine#2156 limit the diagnostic name to `MAX_DIAGNOSTIC_NAME_WIDTH` length
fixes #2156
limit the diagnostic name to
MAX_DIAGNOSTIC_NAME_WIDTH
length