lighthouse
lighthouse copied to clipboard
sigp
First change to validate
Issue Addressed
Fixes #4499
Proposed Changes
Remove the implementation of null_logger, else implement test_logger.
null_logger can be found common/task_executor/src/test_utils.rs
test_logger can be found common/logging/src
Additional Info
I have done only one change as of now. Please review and let me know, whether I am on a right path. Then, I will do changes in other places.
Target branch changed from stable to unstable.
Can you review my changes, and let me know whether I am on the right path or not?
Can you review my changes, and let me know whether I am on the right path or not?
Yep you're on the right path, feel free to proceed!
Heyo, I tried to fix the merge conflicts but I ran afoul of the formatter sorry. Could you please do git pull && cargo fmt and the commit+push the result?
Heyo, I tried to fix the merge conflicts but I ran afoul of the formatter sorry. Could you please do
git pull && cargo fmtand the commit+push the result?
Sure, let me try.
I did git pull and cargo fmt.
Two files got changed in my branch. Shall I commit the cargo.lock ?
Two files got changed in my branch. Shall I commit the cargo.lock ?
I'm not sure, but let's give it a go. Try committing both files and we can see what happens :)
Sorry @postmeback, it looks like committing the Cargo.lock wasn't the right thing to do. To fix it, I'd run:
git checkout unstable Cargo.lock
cargo check
git add Cargo.lock
git commit -m "Refresh Cargo.lock"
git push
I pushed a fix to the Cargo.lock myself, so that's resolved now. However I can see that tests are failing and there are also instances of null_logger still remaining:
grep -rnw '.' -e 'null_logger' --exclude-dir 'target'
./common/task_executor/src/test_utils.rs:29: let log = null_logger().unwrap();
./common/task_executor/src/test_utils.rs:70:pub fn null_logger() -> Result<Logger, String> {
./common/logging/src/lib.rs:236: .expect("Should build null_logger")
./beacon_node/network/src/network_beacon_processor/mod.rs:14:use environment::null_logger;
./beacon_node/network/src/network_beacon_processor/mod.rs:553: let log = null_logger().unwrap();
My advice would be to delete the null_logger() definition in ./common/task_executor/src/test_utils.rs:70: and then go about making sure that you can get both of these commands to run without error:
-
make lint -
make udeps
That should be a surefire way of ensuring there are no more uses of null_logger and it would be a good start to producing something that will pass CI ☺️
Closing for inactivity. Thank you @postmeback for the contribution! Please feel free to pick other good first issues if you want to contribute again