Uh oh!
There was an error while loading. Please reload this page.
bpo-43858: Add logging.getLevelNamesMapping() - #26459
Conversation
ericvsmith
left a comment
There was a problem hiding this comment.
This needs tests. In particular it should add tests that a copy is returned.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot
commented
May 31, 2021
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
akulakov
commented
May 31, 2021
I have made the requested changes; please review again |
bedevere-bot
commented
May 31, 2021
Thanks for making the requested changes! @ericvsmith: please review the changes made to this pull request. |
ericvsmith
left a comment
There was a problem hiding this comment.
This is looking good to me. @vsajip : I'll let you have the final word.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot
commented
May 31, 2021
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1 @@ | |||
| Added a function that returns a copy of a dict of logging levels: func::`logging.getLevelNamesMapping()` | |||
There was a problem hiding this comment.
Now that I look at this: isn't the correct syntax:
:func:`logging.getLevelNamesMapping`
?
There was a problem hiding this comment.
You are right, well spotted!
akulakov
commented
Jun 1, 2021
@vsajip@ericvsmith Pushed an update per comments, please take a look! |
ericvsmith
left a comment
There was a problem hiding this comment.
With the one small item I mentioned, this looks good to me.
| @@ -0,0 +1 @@ | |||
| Added a function that returns a copy of a dict of logging levels: :func:`logging.getLevelNamesMapping()` | |||
There was a problem hiding this comment.
My understanding is that :func: adds the parens, so you should leave them off in this message. I've looked at other files, and they don't have the parens.
akulakov
commented
Jun 1, 2021
@ericvsmith I fixed the parens, please take a look.. |
vsajip
left a comment
There was a problem hiding this comment.
Accidental double-definition in test?
| self.assertEqual(rc, 1) | ||
| def test_get_level_names_mapping(self): | ||
| def test_get_level_names_mapping(self): |
There was a problem hiding this comment.
I don't think you meant this double level of indentation! That causes the test to do nothing effective 😞
There was a problem hiding this comment.
Whoops, somehow doubling made it hard to spot for me..
akulakov
commented
Jun 2, 2021
@vsajip Fixed the doubled test.. |
ericvsmith
left a comment
There was a problem hiding this comment.
This all looks good to me. Thanks, @akulakov !
akulakov
commented
Jun 5, 2021
@ericvsmith and @vsajip much thanks for the help with the PR! |
Added a function that returns a copy of a dict of logging levels.
https://bugs.python.org/issue43858