bpo-39259: nntplib.NNTP/NNTP_SSL refactoring #17939
Conversation
vstinner
changed the title
I prefer to always attach a bpo to get more context. This change is connected to: https://bugs.python.org/issue39259 |
There was a problem hiding this comment.
Hum, you changed how _NNTPBase. With this change, I'm not sure that it's still relevant to have _NNTPBase and NNTP. What do you think of merging _NNTPBase and NNTP into NNTP? The most tricky part is to merge the two __init__() methods. Maybe just move NNTP code at the end of _NNTPBase.init().
|
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 |
|
@vstinner Yes, that's reasonable. I will update this PR with merging 2 classes. |
|
cpython/Lib/test/test_nntplib.py Line 424 in 2e6a8ef Hmm... _NNTPBase is also used for testing server code. |
Lib/nntplib.py
Outdated
| usenetrc, timeout) | ||
|
|
||
| def _create_socket(self, timeout): | ||
| self.sock = super()._create_socket(timeout) |
There was a problem hiding this comment.
In NNTP, the method only creates a socket, it doesn't assign self.sock. Please replace self.sock with sock.
Ok. |
|
@vstinner Updated!
I will create a new issue for this :) |
|
https://bugs.python.org/issue39305 is created :) |
This PR is only refactoring work, so the news is skipped. :)
https://bugs.python.org/issue39259