Skip to content

/ cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
from
Open

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

wants to merge 2 commits into from
+24 −0

Conversation

@corona10
Copy link
Member

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

https://bugs.python.org/issue39259
@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
@corona10 corona10 force-pushed the corona10:bpo-39259-nntplib branch from e1fe47d to 6237928 Jan 10, 2020
@vstinner
vstinner requested changes Jan 10, 2020
View changes
Copy link
Member

vstinner left a comment

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.
Lib/nntplib.py Show resolved Hide resolved
Lib/nntplib.py Outdated Show resolved Hide resolved
Lib/nntplib.py Outdated Show resolved Hide resolved
Lib/nntplib.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

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 corona10:bpo-39259-nntplib branch 2 times, most recently from 1841c55 to 02b3045 Jan 10, 2020
@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 reviewed Jan 10, 2020
View changes
Copy link
Member Author

corona10 left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented Jan 10, 2020

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

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

@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 <donghee.na92@gmail.com>
Date:   Sat Jan 11 00:37:14 2020 +0900

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

commit 0aa665bb0980f81d13480c45ac534235c4d5315a
Author: Dong-hee Na <donghee.na92@gmail.com>
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 <donghee.na92@gmail.com>
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 <donghee.na92@gmail.com>
Date:   Fri Jan 10 23:34:05 2020 +0900

    [bpo-39259](https://bugs.python.org/issue39259): poplib now rejects timeout = 0 (GH-17912)
@corona10 corona10 force-pushed the corona10:bpo-39259-nntplib branch from 38bdc38 to ff07e0a Jan 10, 2020
@vstinner
vstinner requested changes Jan 10, 2020
View changes
Copy link
Member

vstinner left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Jan 10, 2020

@vstinner
Thanks for the guide.
I 've opened a new PR #17939 for the refactoring work.
@corona10 corona10 force-pushed the corona10:bpo-39259-nntplib branch from ff07e0a to 4dc7628 Jan 10, 2020
@corona10

This comment has been minimized.

Sign in to view
Copy link
Member Author

corona10 commented Jan 10, 2020

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

This comment has been minimized.

Sign in to view
Copy link
Member

vstinner commented Jan 11, 2020

I merged your PR #17939. You can now rebase this one on top of it (master branch).
@corona10
nntplib.NNTP/NNTP_SSL refactoring
4d0ea69
@corona10 corona10 force-pushed the corona10:bpo-39259-nntplib branch from 4dc7628 to c435fa9 Jan 11, 2020
@corona10
bpo-39259: nntplib.NNTP/NNTP_SSL now reject timeout = 0
Loading status checks…
beb6021
@corona10 corona10 force-pushed the corona10:bpo-39259-nntplib branch from c435fa9 to beb6021 Jan 11, 2020
@corona10

This comment has been minimized.

Sign in to view
Copy link
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 :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

Assignees
No one assigned
Labels
CLA signed awaiting changes
Projects
None yet
Milestone
No milestone
4 participants
@corona10 @bedevere-bot @vstinner @the-knights-who-say-ni
You can’t perform that action at this time.