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

core: minor fix for the log wrapper with debug purpose #30454

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

CaraWang
Copy link
Contributor

@CaraWang CaraWang commented Sep 18, 2024

After this PR, #28187, the way to set the default logger is different. This PR only updates the way to set logger in some test cases' comments that existed in the codebase (since this commit b63e3c37a6). Although I am not sure if it a good way to leave the code in the comment, it truly makes me more efficiently to debug and fix the failing test cases.

@jwasinger
Copy link
Contributor

IMO these should all just be removed instead of updated. No reason to have commented debug-logging code in the codebase.

@CaraWang
Copy link
Contributor Author

IMO these should all just be removed instead of updated. No reason to have commented debug-logging code in the codebase.

Thank you for your response. =)

At first glance, I thought it was a bad smell. But it's helpful when I need to debug and fix the test cases, so I think it might be helpful for someone else someday. It's okay if these codes should be removed. I can also help remove them in this PR. I will wait for a couple of hours to see if others have different opinions.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change will be useful.

These are very special/complicated tests that dumping logs are occasionally needed for debugging purposes. I would vote to keep/update them.

@rjl493456442 rjl493456442 added this to the 1.14.10 milestone Sep 19, 2024
@rjl493456442 rjl493456442 merged commit c4c2c4f into ethereum:master Sep 19, 2024
3 checks passed
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 this pull request may close these issues.

3 participants