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.

Sign up for GitHub

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

Jump to bottom

Bevy should deny eprintln and println and prefer log and/or tracing #17446

Closed
BenjaminBrienen opened this issue Jan 20, 2025 · 7 comments · Fixed by #18233
Closed

Bevy should deny eprintln and println and prefer log and/or tracing #17446

BenjaminBrienen opened this issue Jan 20, 2025 · 7 comments · Fixed by #18233
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through

Comments

@BenjaminBrienen
Copy link
Contributor

BenjaminBrienen commented Jan 20, 2025 •
edited
Loading

Exactly, I wonder if Bevy should actually prohibit eprintln and println since we always have access to log and/or tracing...

Originally posted by @bushrat011899 in #17442 (comment)
@BenjaminBrienen BenjaminBrienen added A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Jan 20, 2025
@BenjaminBrienen BenjaminBrienen changed the title Exactly, I wonder if Bevy should actually prohibit eprintln and println since we always have access to log and/or tracing... Bevy should prohibit eprintln and println and prefer log and/or tracing Jan 20, 2025
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through A-Cross-Cutting Impacts the entire engine and removed A-Assets Load files from disk to use for things like images, models, and sounds labels Jan 20, 2025
@mockersf
Copy link
Member

mockersf commented Jan 20, 2025 •
edited
Loading

there are some places, mostly parts of bevy_ecs, app initialization or in tests, where we can't assume a logger has been setup. well, and logger setup itself 😄

@bushrat011899
Copy link
Contributor

there are some places, mostly parts of bevy_ecs, app initialization or in tests, where we can't assume a logger has been setup. well, and logger setup itself 😄

Since this should be the exception rather than the rule, perhaps we could deny e/println!, and then explicitly expect its usage in the (hopefully small) number of places it's required?

@mockersf
Copy link
Member

Yup!
println is a big enough trap that it's probably worth it, but it's not as simple as forbid and replace all

@BenjaminBrienen BenjaminBrienen changed the title Bevy should prohibit eprintln and println and prefer log and/or tracing Bevy should deny eprintln and println and prefer log and/or tracing Jan 20, 2025
@BenjaminBrienen
Copy link
Contributor Author

Can you deny macros in deny.toml? If you can, I think you can expect the lint in the exceptional places.

@bushrat011899
Copy link
Contributor

It looks like there's a provided lint, print_stdout that can be used for this, so we don't even need to deny the methods (which as @BenjaminBrienen points out might not be possible since these are macros). I'll need to test to see if it also applies to eprintln too.

@mockersf
Copy link
Member

I'll need to test to see if it also applies to eprintln too.

Probably not as there's a print_stderr lint

@hukasu
Copy link
Contributor

hukasu commented Mar 6, 2025

no_std does not have e/println so it will need to be done at some point

sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
142201f
@sirius94 sirius94 mentioned this issue Mar 10, 2025
Merged
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
a5138bc
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
c87d1d8
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
b5cc909
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
1d428ee
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
8c32b8a
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 10, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
b3592e5
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 11, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
3fbd313
sirius94 added a commit to sirius94/bevy that referenced this issue Mar 11, 2025
@sirius94
Add print_stdout and print_stderr lints (bevyengine#17446)
e5b15f2
github-merge-queue bot pushed a commit that referenced this issue Mar 11, 2025
@sirius94
Add print_stdout and print_stderr lints (#17446) (#18233)
8570af1 
# Objective

- Prevent usage of `println!`, `eprintln!` and the like because they
require `std`
- Fixes #17446

## Solution

- Enable the `print_stdout` and `print_stderr` clippy lints
- Replace all `println!` and `eprintln!` occurrences with `log::*` where
applicable or alternatively ignore the warnings

## Testing

- Run `cargo clippy --workspace` to ensure that there are no warnings
relating to printing to `stdout` or `stderr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add print_stdout and print_stderr lints (#17446) sirius94/bevy
5 participants
@bushrat011899 @alice-i-cecile @hukasu @mockersf @BenjaminBrienen