-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
simple custom log macro to avoid additional dependencies #6315
Conversation
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.
- I still don't see a usecase, and
- this would cause completely unnecessary computation/allocation when logging is switched off, and
- it doesn't address the main usecase that I'm guessing ("want to debug in an eprintln-style during tests that check for stderr").
42afcc4
to
b738a27
Compare
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.
I still don't quite see the advantage of this. So you're suggesting that we change:
- old strategy: just insert eprintln!() temporarily whenever you debug something, and delete it before pushing (and if you forget, CI will remind you)
- new strategy: just insert uu_log!() temporarily whenever you debug something, and delete it before pushing (and if you forget, a weird environment variable will cause file writes in production that GNU doesn't do)
So what is the advantage then?
EDIT: Ah, I was hoping this would reset the "changes requested" status. I'm neutral on this PR, and don't understand how to tell github about it.
GNU testsuite comparison:
|
ad704ba
to
4be36ac
Compare
Thanks for neutral rating. I'd formulate it like this:
@sylvestre Can you please review this proposal. I think complexity-wise you probably would rather agree here. |
4be36ac
to
4124d9d
Compare
GNU testsuite comparison:
|
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.
NOT AN APPROVAL, but I don't find a way to reset the "changes requested" status, so I do it like this.
I am sorry but I still don't see the benefit of logging in general ?! |
GNU testsuite comparison:
|
There are no remaining negative side effects left that I know of. |
Sure but you are repeating the what when I care about the why? |
GNU testsuite comparison:
|
I apprechiate your quick response. |
draft 3 for #6284, alternative for #6313 and #6314
The main idea here is that we have a own log macro to avoid additional dependencies.
Same as with #6314, the idea is that the logs are not part of the stable code-base.
They would always be removed when debuggin is done.
This way, there is no explicit activation mechanism needed and the test-framework
can always specify the logfile-path via environmental variable.
It will check after execution if the logfile was written to and then dumps the content.