Stop sending debug strings to the client#445
Conversation
rustaceanrob
force-pushed
the
9-3-rm-debug
branch
from
f93096b to
0fcc5bb
Compare
|
This is not a direct review of this PR, but just chiming in: in general I agree that whatever is considered "debug" logs should not really need to leave the scope of the piece of software using it. I haven't looked at all debug strings that get transmitted, but I assume if we ended up needing any of that at the client layer then it might be best provided through more appropriate channels (or just INFO logs). |
src/lib.rs
Outdated
| } | ||
| macro_rules! debug { | ||
| ($expr:expr) => { | ||
| #[cfg(test)] |
There was a problem hiding this comment.
Would it be more helpful to hook into the debug_assertions so these are emitted for debug builds? Glancing through, I think they would be nice for maintainers and users of the lib.
Lazily just asking the llm on this:
macro_rules! debug {
($expr:expr) => {
#[cfg(debug_assertions)]
eprintln!("[kyoto] {}", $expr)
};
}There was a problem hiding this comment.
In a prior commit I actually used this macro. I was thinking it might be too much unexpected chatter, but it also applies to tests. Maybe it makes sense to go this route and retract it later? Do you think the test cfg is too hacky?
There was a problem hiding this comment.
I was thinking of when running examples to test something, I'd probably want to see the debug chatter vs. having to setup a test to see it.
rustaceanrob
force-pushed
the
9-3-rm-debug
branch
from
0fcc5bb to
fccbed6
Compare
Now that there are some initial users with `bitcoin-safe` and the BDKSwiftExampleApp, I have seen a few integrations and found some glaring problems with the API. In practice, the debug (`LogLevel::Debug`) strings are not seen by the end-user, and developers are not using them either. When they have been used, I have seen that developers are parsing these strings for information. That would imply these strings are "versioned" in a sense. These should be removed ASAP and only accessable by developers of this repository. I believe these are still useful in the debugging process during testing, so I have these strings emitted under `cfg(test)`. A little hacky, but it serves the purpose as what these debug strings have typically been used for.
rustaceanrob
force-pushed
the
9-3-rm-debug
branch
from
fccbed6 to
9dfe8cf
Compare
3 participants
First commit explains the logic for this change.
Any thoughts on this @nyonson?