Skip to content

bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0#17936

Merged
vstinner merged 2 commits intopython:masterfrom
corona10:bpo-39259-nntplib
Jan 11, 2020
Merged

bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0#17936
vstinner merged 2 commits intopython:masterfrom
corona10:bpo-39259-nntplib

Conversation

@corona10
Copy link
Copy Markdown
Member

@corona10 corona10 commented Jan 10, 2020 •
edited by bedevere-bot
Loading

@corona10 corona10 changed the title bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 [WIP] bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 Jan 10, 2020
vstinner
vstinner requested changes Jan 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactoring is interesting. I merged your other change to reject timeout=0, you should now rebase this PR.

Currently, this PR is a refactoring and rejects timeout=0. I prefer to have one commit per change.

Comment thread Lib/nntplib.py Outdated
Comment thread Lib/nntplib.py Outdated
Comment thread Lib/nntplib.py Outdated
Comment thread Lib/nntplib.py Outdated
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jan 10, 2020

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@corona10 corona10 force-pushed the bpo-39259-nntplib branch 2 times, most recently from 1841c55 to 02b3045 Compare January 10, 2020 15:29
@corona10 corona10 changed the title [WIP] bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 Jan 10, 2020
corona10
corona10 commented Jan 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member Author

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the requested changes; please review again

Thanks for the review 👍

  • I 've separated commits :)
  • Also, applied feedback from code reviews, Thank you very much!

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Jan 10, 2020

You should now rebase your PR on master, since I merged your other PR.

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Jan 10, 2020 •
edited by bedevere-bot
Loading

@vstinner
Hmm.. I already rebase this PR.


commit 38bdc38ff90fb058ae80a78ca0ecf0096f4e616c (HEAD -> [bpo-39259](https://bugs.python.org/issue39259)-nntplib, corona10/[bpo-39259](https://bugs.python.org/issue39259)-nntplib)
Author: Dong-hee Na <[email protected]>
Date:   Sat Jan 11 00:37:14 2020 +0900

    [bpo-39259](https://bugs.python.org/issue39259): Update

commit 0aa665bb0980f81d13480c45ac534235c4d5315a
Author: Dong-hee Na <[email protected]>
Date:   Sat Jan 11 00:33:02 2020 +0900

    b[bpo-39259](https://bugs.python.org/issue39259): nntplib.NNTP/NNTP_SSL now reject timeout = 0

commit 02b3045015efb1bbb9dbb314fc2217378d4c6b32
Author: Dong-hee Na <[email protected]>
Date:   Fri Jan 10 23:27:45 2020 +0900

    [bpo-39259](https://bugs.python.org/issue39259): nntplib.NNTP/NNTP_SSL refactoring

commit c39b52f1527868c7ada9385669c38edf98858921 (origin/master)
Author: Dong-hee Na <[email protected]>
Date:   Fri Jan 10 23:34:05 2020 +0900

    [bpo-39259](https://bugs.python.org/issue39259): poplib now rejects timeout = 0 (GH-17912)

vstinner
vstinner requested changes Jan 10, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooooh, now I get it :-) I merged a PR to modify poplib, but this is a PR for nntplib :-D

Please split your PR to extract the _create_socket() refactoring. I would like to first have a PR for the refactoring, and then have a PR to reject timeout=0. Having a single PR for two changes makes review harder. In Python, we always squash multiple commits into a single commit on merge.

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Jan 10, 2020

@vstinner
Thanks for the guide.
I 've opened a new PR #17939 for the refactoring work.

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Jan 10, 2020

So this PR should be reviewed after #17939 is merged.

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Jan 11, 2020

I merged your PR #17939. You can now rebase this one on top of it (master branch).

@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Jan 11, 2020

I merged your PR #17939. You can now rebase this one on top of it (master branch).

Thanks! I 've rebased the PR :)

@vstinner vstinner merged commit 1b335ae into python:master Jan 11, 2020
@vstinner
Copy link
Copy Markdown
Member

vstinner commented Jan 11, 2020

Thanks to the refactoring, it was way easier to review this PR. It's now merged, thanks!

sthagen added a commit to sthagen/python-cpython that referenced this pull request Jan 11, 2020
@sthagen
Merge pull request #162 from python/master
77c6dd8 
bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 (pythonGH-17936)
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@corona10 @shihai1991
bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0 (pythonGH-17936)
2f1ff76 
nntplib.NNTP and nntplib.NNTP_SSL now raise a ValueError
if the given timeout for their constructor is zero to
prevent the creation of a non-blocking socket.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@corona10 @bedevere-bot @vstinner @the-knights-who-say-ni