Skip to content
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

Prefer Display over Debug #16112

Merged
merged 20 commits into from
Dec 27, 2024

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Oct 26, 2024

Objective

Fixes #16104

Solution

I removed all instances of :? and put them back one by one where it caused an error.

I removed some bevy_utils helper functions that were only used in 2 places and don't add value. See: #11478

Testing

CI should catch the mistakes

Migration Guide

bevy::utils::{dbg,info,warn,error} were removed. Use bevy::utils::tracing::{debug,info,warn,error} instead.

@BenjaminBrienen BenjaminBrienen self-assigned this Oct 26, 2024
@BenjaminBrienen BenjaminBrienen added A-Diagnostics Logging, crash handling, error reporting and performance analysis D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 26, 2024
@BenjaminBrienen
Copy link
Contributor Author

@SpecificProtagonist feel free to review or point out good places to use disqualified::ShortName here.

@BenjaminBrienen BenjaminBrienen changed the title Prefer Display over Debug Prefer Display over Debug and format args consistently Oct 26, 2024
@mockersf
Copy link
Member

Why is there a migration guide? There's no information in the description about this change.

I would prefer this to be blocked until after the 0.15 is released, as it has high chances of conflicts during cherry picks.

@mockersf
Copy link
Member

Controversial by extension of #16113 as it was used to do this PR

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Oct 26, 2024
@SpecificProtagonist
Copy link
Contributor

I'm not the biggest fan of mixing widespread code formatting changes with changes in behavior. Did you do DebugDisplay switches for anything besides Entity (and str)?

feel free to review or point out good places to use disqualified::ShortName here.

I'll do that, or maybe I'll change them myself – but again, the PR probably shouldn't include both the code format changes too. But before that: Is there any criteria according to which we decide whether to use the short name in messages? E.g. I think the full name is useful when referring to systems.

@bushrat011899
Copy link
Contributor

I agree with the changes made here, but I don't think it's worth it unless we have some lints to enforce the new standard. If we had format_debug_instead_of_display and format_parameter_instead_of_inline (made-up names) then we'd be in a better position to make this change long-term. Right now, this change will be made and then almost immediately regressed, since we have neither tooling nor policy to enforce it.

@BenjaminBrienen
Copy link
Contributor Author

I agree with the changes made here, but I don't think it's worth it unless we have some lints to enforce the new standard. If we had format_debug_instead_of_display and format_parameter_instead_of_inline (made-up names) then we'd be in a better position to make this change long-term. Right now, this change will be made and then almost immediately regressed, since we have neither tooling nor policy to enforce it.

There is https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow%2Cwarn%2Cnone&groups=nursery%2Cpedantic%2Crestriction%2Cstyle#uninlined_format_args

@bushrat011899
Copy link
Contributor

Oh perfect! I would spin off just the addition of that lint into its own PR first, since that's enforceable. If there's an equivalent lint for the Debug/Display then likewise.

@BenjaminBrienen BenjaminBrienen changed the title Prefer Display over Debug and format args consistently Prefer Display over Debug Oct 28, 2024
@BenjaminBrienen BenjaminBrienen removed the X-Controversial There is active debate or serious implications around merging this PR label Oct 28, 2024
@BenjaminBrienen
Copy link
Contributor Author

I refined this PR to not have the formatting changes

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

I think this is good, but as stated I would open an issue for a lint which keeps this change maintained long-term.

Copy link
Contributor

@SpecificProtagonist SpecificProtagonist left a comment

Choose a reason for hiding this comment

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

Nice, this is much easier to review now!

I haven't seen any discussion of whether to use Debug or Display for entities – the former includes the raw bits, because that matches the serialized format, while the later is more concise.

crates/bevy_ecs/src/system/mod.rs Outdated Show resolved Hide resolved
crates/bevy_utils/src/lib.rs Outdated Show resolved Hide resolved
examples/ecs/system_piping.rs Outdated Show resolved Hide resolved
Copy link
Member

@mnmaita mnmaita left a comment

Choose a reason for hiding this comment

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

Nice one, leaving two minor suggestions.

crates/bevy_ui/src/layout/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/mod.rs Outdated Show resolved Hide resolved
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 9, 2024
@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 26, 2024
@BenjaminBrienen BenjaminBrienen added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Dec 26, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 26, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 26, 2024
@alice-i-cecile
Copy link
Member

@BenjaminBrienen CI failures are real; I think you need to revert a couple of things here.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 27, 2024
Merged via the queue into bevyengine:main with commit 64efd08 Dec 27, 2024
28 checks passed
@BenjaminBrienen BenjaminBrienen deleted the consistent-formatting branch December 27, 2024 00:57
@BenjaminBrienen BenjaminBrienen removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent formatting of entities in error messages
6 participants