-
Notifications
You must be signed in to change notification settings - Fork 670
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
Fix logs to use inplace formatting in stacks node #5419
Conversation
Signed-off-by: Jacinta Ferrant <[email protected]>
If we're going to do this, maybe we should also use named parameters when the expression cannot be directly inserted in place. I find it a little less clear when there is a mix of empty test_debug!(
"Miner {miner_id}: Block commit transaction builds on {parent_block_ptr},{parent_vtxindex} (parent snapshot is {parent_block_snapshot_opt:?})",
miner_id = miner.id,
parent_block_ptr = block_commit_op.parent_block_ptr,
parent_vtxindex = block_commit_op.parent_vtxindex
); |
I agree. I think I should do this as well. I am doing it as a blanket apply across all code, but don't think we have to be super strict about this. By no means should this prevent a PR merging, but I think we as a team should try introducing it as a best practice :) |
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.
Approving provided all CI tests pass.
This was more of a just try it out to see if its an improvement. There are many instances where I think it is. Should we introduce this as a general standard from now on?