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

Allow callers to enable to use of select.poll() #474

Merged
merged 4 commits into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@cooperlees
Contributor

cooperlees commented Mar 23, 2018

  • Add the use_poll bool for callers to choose if they want poll
  • This ensure we leave the default as it is today
  • Added unit tests to use select.poll()
  • Updated documentation guessing format etc.

If wrong I can fix, or if you could just fix post merge that would be amazing :D

cooperlees added some commits Mar 23, 2018

Allow for configurable select.poll() usage
- This allows systems that can have > 1024 fds to work
- Should be marginally faster
- Added tests to use select.poll()
- Incremented version - Happy to change this, guessed what it should be
@Red-M

This comment has been minimized.

Member

Red-M commented Mar 26, 2018

Can I ask that if you're updating the release changelog that I can put pxssh's release changes in first?
I have a few new features planned and I don't want to break your PR here.

@cooperlees

This comment has been minimized.

Contributor

cooperlees commented Mar 26, 2018

I don't 100% follow what you want me to do? Just wait and rebase yes? I guess I should also get the test coverage back up.

poller = select.poll()
for fd in fds:
poller.register(fd)
while True:

This comment has been minimized.

@takluyver

takluyver Mar 26, 2018

Member

This should probably be outside the for loop, if I'm understanding it correctly.

This comment has been minimized.

@Red-M

Red-M Mar 30, 2018

Member

Before I merge this, can you confirm that this is what you want to do?

This comment has been minimized.

@cooperlees

cooperlees Mar 30, 2018

Contributor

I want to register each fd right or am I misunderstanding what I need to do here? I guess we're only really ever passing 1 FD tho ...

This comment has been minimized.

@Red-M

Red-M Mar 30, 2018

Member

you never break the while loop and it has no condition to allow the next fd to come into play.

This comment has been minimized.

@takluyver

takluyver Mar 30, 2018

Member

I think the while loop should be dedented one level so it happens after the for loop instead of inside it.

It doesn't make a difference when we only pass one fd, but it will be broken if the function is ever used with more than one, so we might as well get it right.

@takluyver

This comment has been minimized.

Member

takluyver commented Mar 26, 2018

This looks OK to me.

Release notes are always a pain when there are multiple changes in flight at the same time, because they guarantee conflicts. Tools like towncrier can help, but it's probably overkill to use that for a relatively quiet project like Pexpect.

@Red-M

This comment has been minimized.

Member

Red-M commented Mar 30, 2018

I'm ok to merge this.
I'll do another PR to add my release notes in for pxssh :)

@cooperlees

This comment has been minimized.

Contributor

cooperlees commented Mar 31, 2018

changed the API to take one fd only more like select_ignore_interrupts

@Red-M

This comment has been minimized.

Member

Red-M commented Mar 31, 2018

Seems fine to me, if @takluyver has no comments I'll merge this if he doesn't.

* :class:`~.spawn` now has a ``use_poll`` parameter. This allows the use
of select.poll() on fds
* :class:`~.fdspawn` now has a ``use_poll`` paramneter. This allow the use
of select.poll() on fds

This comment has been minimized.

@takluyver

takluyver Mar 31, 2018

Member

Let's combine these two notes into one (it's one conceptual change, even if it affects two classes) and mention that:

  • It lets you get round problems with file descriptors > 1024.
  • It's not used by default because of compatibility concerns, so the downstream code has to decide to use it.

This comment has been minimized.

@takluyver

takluyver Apr 8, 2018

Member

Ping @cooperlees ? If you can make these changes to the docs, then I think the PR is ready to merge.

def poll_ignore_interrupts(fd, timeout=None):
"""Simple wrapper around poll for a list of file descriptors."""

This comment has been minimized.

@takluyver

takluyver Mar 31, 2018

Member

Don't forget to update the docstring now that it only takes one fd.

@cooperlees

This comment has been minimized.

Contributor

cooperlees commented Apr 9, 2018

Updated documentation. I do feel it would of been faster for you to just merge and fix up the documentation to how you want it. A new comer is never going to get documentation how main mergers like it. I hope it's all fine now.

@Red-M Red-M merged commit cad8b12 into pexpect:master Apr 9, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.5%) to 88.608%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Member

takluyver commented Apr 11, 2018

@cooperlees sorry, I just noticed that there's a problem in this code - in __interact_copy it needs to handle two fds (like the select implementation does). Have you got time to take a look?

@cooperlees

This comment has been minimized.

Contributor

cooperlees commented Apr 12, 2018

Will take a look tomorrow.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Apr 12, 2018

It's important for contributors to make the necessary documentation changes. When someone has a question about the documentation, they will look at the git blame output and contact the person who committed the changes. Also, while it might be faster for someone else to make the changes, contributors won't learn what is expected if they don't make the changes themselves.

@cooperlees

This comment has been minimized.

Contributor

cooperlees commented Apr 13, 2018

@ryanpetrello

This comment has been minimized.

Contributor

ryanpetrello commented May 21, 2018

@cooperlees @shoyer it doesn't seem like this implementation respects the timeout argument to spawn(): #491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment