bpo-41177: Add iterator support to ConvertingList, ConvertingTuple, and items() to ConvertingDict#21248
Conversation
- ConvertingDict - ConvertingList - ConvertingTuple
|
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). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
vsajip
changed the title
vsajip
added
the
🔨 test-with-buildbots
bedevere-bot
removed
the
🔨 test-with-buildbots
vsajip
left a comment
vsajip
left a comment
There was a problem hiding this comment.
I think tests should be added, but the NEWS file removed (as the APIs being changed are internal, they don't need a NEWS entry - I'll add the skip-news label so it doesn't cause a block to merging).
|
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 |
| # Can't replace a tuple entry. | ||
| return self.convert_with_key(key, value, replace=False) | ||
|
|
||
| def __iter__(self): |
There was a problem hiding this comment.
Is it needed? Are any tests failed without it?
There was a problem hiding this comment.
No tests fail without it, but the request is for additional functionality (which would need its own tests, of course). The issue gives more info about the use case.
There was a problem hiding this comment.
I suppose that tests for iterating ConvertingList and ConvertingTuple would expose that there is no need to add __iter__().
Tests are necessary.
|
I requested that tests be added, but the OP has not responded to that. The requested functionality could be added (the use case in the issue is not unreasonable) but it's not an especial priority - the OP can implement tests if they want to hurry things along, and I'll review it. |
|
The original issue has been closed. |
5 participants
Add iterator support for logging.config.ConvertingList and logging.config.ConvertingTuple
Add items() support for logging.config.ConvertingDict.
https://bugs.python.org/issue41177