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

Fail on RUST_LOG parse error #1434

Closed
zthompson47 opened this issue Dec 9, 2022 · 1 comment · Fixed by #1435
Closed

Fail on RUST_LOG parse error #1434

zthompson47 opened this issue Dec 9, 2022 · 1 comment · Fixed by #1435

Comments

@zthompson47
Copy link
Contributor

At the top of crates/fj-app/src/main.rs there's this comment:

// It would be better to fail, if `RUST_LOG` is erroneous, but I don't know
// how to distinguish between that and the "not defined" case.

I figured out how to detect if RUST_LOG is not defined and came up with a PR that I'll attach to this issue.

My solution terminates the program with an error message if there is any type of parse error. That includes not just parsing errors in tracing_subscriber, but also the NotUnicode variant of std::env::VarError. It succeeds only if the parsing is correct or if RUST_LOG is not defined (the latter case defaults to WARN). Hopefully that's what you had in mind, but I can always revise.

I didn't write a test for this change, but I'd be happy to work on that if you think it's warranted. I feel like it's something to just manually check and assume it will continue working. That said, I've only checked the obvious cases, and e.g. haven't tested a non-unicode RUST_LOG. I guess I'm just relying on the compiler guarantees. Please advise if my thinking is off..

zthompson47 added a commit to zthompson47/Fornjot that referenced this issue Dec 9, 2022
Default to WARN log level if RUST_LOG is not defined or else terminate
the program with an error message. Fixes hannobraun#1434
@hannobraun
Copy link
Owner

Again, thank you, @zthompson47!

Hopefully that's what you had in mind, but I can always revise.

Yes, that's exactly it 👍

I didn't write a test for this change, but I'd be happy to work on that if you think it's warranted. I feel like it's something to just manually check and assume it will continue working. That said, I've only checked the obvious cases, and e.g. haven't tested a non-unicode RUST_LOG. I guess I'm just relying on the compiler guarantees. Please advise if my thinking is off..

I think that tracks. If you had presented me with a test, I'd have happily merged that, but I don't think it would be worth the effort to write that test, probably.

Maybe that will change over time, as Fornjot gets more mature. But that would also mean we'd have to put infrastructure in place to make that kind of testing straight-forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants