bpo-38361: syslog: fixed making default "ident" from sys.argv[0]#16557
Conversation
The default value of "ident" parameter should be sys.argv[0] with leading path components stripped, but it contained the last slash, i.e. '/program' instead of 'program'.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
vaclavbartos
changed the title
|
Thanks for your time @vaclavbartos, and welcome to CPython! 😎 This looks like a good change. I assume you’ve already seen the bot’s message about the CLA? |
brandtbucher
left a comment
brandtbucher
left a comment
There was a problem hiding this comment.
Even though this is a fairly trivial fix, I still think you should make a NEWS entry, since it changes logging output. Just something simple, like:
Fixed an issue where ``ident`` could include a leading path separator when :func:`syslog.openlog` was called without arguments.And while a regression test or two are always good to include, it looks like this module has almost no test coverage currently due to platform issues. So, probably not necessary in this case.
|
OK, I added the News entry. I signed the CLA yesterday and it's checked now. Regarding tests - as you noted, it would be quite difficult to implement them, since there is currently no other way to read |
|
This module doesn't have any experts with GitHub accounts, and hasn't had any real changes in almost a decade. Given that this is a fairly trivial change, perhaps another core dev could look at it? CC @vstinner |
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
Yeah, this looks good. I agree that this looks like a mis-translation of the original code, apparently introduced in revision d63a3b8.
|
Should we backport this to 3.8? Then the behavior would be different in 3.8.1 than in 3.8.0. |
|
Even though it's a bug fix, I'm leaning "no". Logging output shouldn't change between minor versions, and I don't know of anyone who sees this as a must-have. It's just another reason 3.9 will be better than 3.8. |
|
Going through some of my old reviews and saw that this was still unmerged. @gvanrossum, is this waiting for anything? |
|
Sorry! It should be merged now. |
|
Congrats on your first CPython contribution @vaclavbartos! 🍾 Looking forward to seeing more from you in the future. |
…honGH-16557) The default value of "ident" parameter should be sys.argv[0] with leading path components stripped, but it contained the last slash, i.e. '/program' instead of 'program'. BPO issue: https://bugs.python.org/issue38361 https://bugs.python.org/issue38361
6 participants
The default value of "ident" parameter should be sys.argv[0] with leading path
components stripped, but it contained the last slash, i.e. '/program' instead
of 'program'.
BPO issue: https://bugs.python.org/issue38361
https://bugs.python.org/issue38361
Automerge-Triggered-By: @gvanrossum