Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add py2 stubs for multiprocessing.Queue #1829

Merged
merged 2 commits into from Jan 18, 2018
Merged

Add py2 stubs for multiprocessing.Queue #1829

merged 2 commits into from Jan 18, 2018

Conversation

dzbarsky
Copy link
Contributor

No description provided.

ilevkivskyi
ilevkivskyi reviewed Jan 16, 2018
View reviewed changes
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thank you for PR!

Only have one small comment.

stdlib/2/multiprocessing/__init__.pyi Outdated

class ProcessError(Exception): ...
class BufferTooShort(ProcessError): ...
class TimeoutError(ProcessError): ...
class AuthenticationError(ProcessError): ...

_T = TypeVar('_T')

class Queue(queue.Queue[_T]):
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this start with capital "Q" on Python 2?

ilevkivskyi
ilevkivskyi reviewed Jan 16, 2018
View reviewed changes
stdlib/2/multiprocessing/__init__.pyi Outdated
from multiprocessing.process import Process as Process, current_process as current_process, active_children as active_children
from multiprocessing.util import SUBDEBUG as SUBDEBUG, SUBWARNING as SUBWARNING
import Queue as queue
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be underscored (either _queue or _Queue as you wish). Otherwise type checkers will treat queue as externally visible (i.e. imported for re-export). You can read about special nature of ... as ... imports in stubs in PEP 484, if interested.

@gvanrossum
Copy link
Member

Please don't squash commits after review has started.

ilevkivskyi
ilevkivskyi approved these changes Jan 16, 2018
View reviewed changes
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks!

LGTM now.

@dzbarsky
Copy link
Contributor Author

Sorry about squashing, I assume we would want to squash anyway before merging and figured the PR was small enough that it wouldn't be confusing.

Will this get auto-merged when travis runs?

@ilevkivskyi
Copy link
Member

Will this get auto-merged when travis runs?

I will take care.

@gvanrossum
Copy link
Member

We never auto-merge, we always squash-merge.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Jan 17, 2018 •
edited by gvanrossum

Heh, this is why it is good to wait for all tests to pass.
@dzbarsky There are two problems:

  • One is easy, you forgot to import TypeVar from typing (sorry I didn't notice it either).
  • Another is hard, pytype doesn't like the module alias, @matthiaskramm could you please advise on this?

@gvanrossum
Copy link
Member

The solution is presumably from Queue import Queue as _BaseQueue.

@ilevkivskyi ilevkivskyi merged commit 7f3e015 into python:master Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dzbarsky @gvanrossum @ilevkivskyi