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

Update Quamash to work with Python 3.4 asyncio #2

Merged
merged 7 commits into from
Jul 11, 2014
Merged

Update Quamash to work with Python 3.4 asyncio #2

merged 7 commits into from
Jul 11, 2014

Conversation

aknuds1
Copy link
Collaborator

@aknuds1 aknuds1 commented Jul 1, 2014

Hi Mark

I have re-written Quamash to make it work with Python 3.4 asyncio and PyQt 5/PySide. Note that I have only tested with PyQt 5 on Windows so far, and what I have tested is reflected in the automatic tests I've written (in the tests/ directory). Not sure how easy it would be to support older versions of Python. This was my first exposure to Python async, except Twisted.

@harvimt
Copy link
Owner

harvimt commented Jul 1, 2014

I actually think I had it working in py 3.4 but was having problems getting
it to work on windows. I'm on my laptop right now so I don't have access to
any of those working copies though.

Just a quick few things, If I'm going to maintain this, you need to use
tabs (I realize I'm a heretic), but the diffs looks terrible.

And your IDEA IDE stuff should be .gitignored.

Really the tabs things makes it hard for me to tell exactly all that has
changed, since everything has changed.

But it does look good, I'll take a closer look at it this evening (Pacific
Time).

On Tue, Jul 1, 2014 at 6:19 AM, Arve Knudsen notifications@github.com
wrote:

Hi Mark

I have re-written Quamash to make it work with Python 3.4 asyncio and PyQt
5/PySide. Note that I have only tested with PyQt 5 on Windows so far, and
what I have tested is reflected in the automatic tests I've written (in the
tests/ directory). Not sure how easy it would be to support older versions

of Python. This was my first exposure to Python async, except Twisted.

You can merge this Pull Request by running

git pull https://github.com/aknuds1/quamash master

Or view, comment on, or merge it at:

#2
Commit Summary

  • Update to 3.4 asyncio API
  • Update README
  • Fix README
  • Link to PEP
  • Base implementation on standard ProactorEventLoop/SelectorEventLoop
  • Add license
  • Fix event loop close issues
  • Merge branch 'master' into proactor
  • Add logging
  • Fix some module refereces
  • Fix README example
  • More logging
  • Fix run_until_complete
  • Add travis configuration
  • Fix(?) Travis config
  • Fix context manager logic
  • Support PySide
  • Implement subprocess_exec

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#2.

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 1, 2014

Hi

Actually, mine is broken on Unix for the subprocess stuff. I'll have to
make some of the same tweaks I did for the WIndows version for Unix as
well. It's probably not very difficult. I didn't realize you were using
tabs, I think this is the very first time I've come across this in a
project :) I'll ignore the IDEA stuff, not a problem.

Arve

On Tue, Jul 1, 2014 at 7:34 PM, Mark Harviston notifications@github.com
wrote:

I actually think I had it working in py 3.4 but was having problems
getting
it to work on windows. I'm on my laptop right now so I don't have access
to
any of those working copies though.

Just a quick few things, If I'm going to maintain this, you need to use
tabs (I realize I'm a heretic), but the diffs looks terrible.

And your IDEA IDE stuff should be .gitignored.

Really the tabs things makes it hard for me to tell exactly all that has
changed, since everything has changed.

But it does look good, I'll take a closer look at it this evening (Pacific
Time).

On Tue, Jul 1, 2014 at 6:19 AM, Arve Knudsen notifications@github.com
wrote:

Hi Mark

I have re-written Quamash to make it work with Python 3.4 asyncio and
PyQt
5/PySide. Note that I have only tested with PyQt 5 on Windows so far,
and
what I have tested is reflected in the automatic tests I've written (in
the
tests/ directory). Not sure how easy it would be to support older
versions

of Python. This was my first exposure to Python async, except Twisted.

You can merge this Pull Request by running

git pull https://github.com/aknuds1/quamash master

Or view, comment on, or merge it at:

#2
Commit Summary

  • Update to 3.4 asyncio API
  • Update README
  • Fix README
  • Link to PEP
  • Base implementation on standard ProactorEventLoop/SelectorEventLoop
  • Add license
  • Fix event loop close issues
  • Merge branch 'master' into proactor
  • Add logging
  • Fix some module refereces
  • Fix README example
  • More logging
  • Fix run_until_complete
  • Add travis configuration
  • Fix(?) Travis config
  • Fix context manager logic
  • Support PySide
  • Implement subprocess_exec

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#2.

—
Reply to this email directly or view it on GitHub
#2 (comment).

@harvimt
Copy link
Owner

harvimt commented Jul 1, 2014

tabs-using python programmers are rare, but we exist, and we didn't get
this way by not being stubborn.

:-)

On Tue, Jul 1, 2014 at 10:40 AM, Arve Knudsen notifications@github.com
wrote:

Hi

Actually, mine is broken on Unix for the subprocess stuff. I'll have to
make some of the same tweaks I did for the WIndows version for Unix as
well. It's probably not very difficult. I didn't realize you were using
tabs, I think this is the very first time I've come across this in a
project :) I'll ignore the IDEA stuff, not a problem.

Arve

On Tue, Jul 1, 2014 at 7:34 PM, Mark Harviston notifications@github.com
wrote:

I actually think I had it working in py 3.4 but was having problems
getting
it to work on windows. I'm on my laptop right now so I don't have access
to
any of those working copies though.

Just a quick few things, If I'm going to maintain this, you need to use
tabs (I realize I'm a heretic), but the diffs looks terrible.

And your IDEA IDE stuff should be .gitignored.

Really the tabs things makes it hard for me to tell exactly all that has
changed, since everything has changed.

But it does look good, I'll take a closer look at it this evening
(Pacific
Time).

On Tue, Jul 1, 2014 at 6:19 AM, Arve Knudsen notifications@github.com
wrote:

Hi Mark

I have re-written Quamash to make it work with Python 3.4 asyncio and
PyQt
5/PySide. Note that I have only tested with PyQt 5 on Windows so far,
and
what I have tested is reflected in the automatic tests I've written
(in
the
tests/ directory). Not sure how easy it would be to support older
versions

of Python. This was my first exposure to Python async, except Twisted.

You can merge this Pull Request by running

git pull https://github.com/aknuds1/quamash master

Or view, comment on, or merge it at:

#2
Commit Summary

  • Update to 3.4 asyncio API
  • Update README
  • Fix README
  • Link to PEP
  • Base implementation on standard ProactorEventLoop/SelectorEventLoop
  • Add license
  • Fix event loop close issues
  • Merge branch 'master' into proactor
  • Add logging
  • Fix some module refereces
  • Fix README example
  • More logging
  • Fix run_until_complete
  • Add travis configuration
  • Fix(?) Travis config
  • Fix context manager logic
  • Support PySide
  • Implement subprocess_exec

File Changes

(23)

(53)

Patch Links:

—
Reply to this email directly or view it on GitHub
#2.

—
Reply to this email directly or view it on GitHub
#2 (comment).

—
Reply to this email directly or view it on GitHub
#2 (comment).

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 1, 2014

I've made a 'unix' (https://github.com/aknuds1/quamash/tree/unix) branch to
start support for Unix. I've tested on OS X so far, and launching a
subprocess just hangs the event loop :( I figure some good old fashioned
debugging will be required to figure out what's misbehaving. I don't have
the time tonight though, I know it can be a time consuming process, as I
spent hours today figuring out how it works on Windows.

Arve

On Tue, Jul 1, 2014 at 7:44 PM, Mark Harviston notifications@github.com
wrote:

tabs-using python programmers are rare, but we exist, and we didn't get
this way by not being stubborn.

:-)

On Tue, Jul 1, 2014 at 10:40 AM, Arve Knudsen notifications@github.com
wrote:

Hi

Actually, mine is broken on Unix for the subprocess stuff. I'll have to
make some of the same tweaks I did for the WIndows version for Unix as
well. It's probably not very difficult. I didn't realize you were using
tabs, I think this is the very first time I've come across this in a
project :) I'll ignore the IDEA stuff, not a problem.

Arve

On Tue, Jul 1, 2014 at 7:34 PM, Mark Harviston notifications@github.com

wrote:

I actually think I had it working in py 3.4 but was having problems
getting
it to work on windows. I'm on my laptop right now so I don't have
access
to
any of those working copies though.

Just a quick few things, If I'm going to maintain this, you need to
use
tabs (I realize I'm a heretic), but the diffs looks terrible.

And your IDEA IDE stuff should be .gitignored.

Really the tabs things makes it hard for me to tell exactly all that
has
changed, since everything has changed.

But it does look good, I'll take a closer look at it this evening
(Pacific
Time).

On Tue, Jul 1, 2014 at 6:19 AM, Arve Knudsen notifications@github.com

wrote:

Hi Mark

I have re-written Quamash to make it work with Python 3.4 asyncio
and
PyQt
5/PySide. Note that I have only tested with PyQt 5 on Windows so
far,
and
what I have tested is reflected in the automatic tests I've written
(in
the
tests/ directory). Not sure how easy it would be to support older
versions
of Python. This was my first exposure to Python async, except

Twisted.

You can merge this Pull Request by running

git pull https://github.com/aknuds1/quamash master

Or view, comment on, or merge it at:

#2
Commit Summary

  • Update to 3.4 asyncio API
  • Update README
  • Fix README
  • Link to PEP
  • Base implementation on standard
    ProactorEventLoop/SelectorEventLoop
  • Add license
  • Fix event loop close issues
  • Merge branch 'master' into proactor
  • Add logging
  • Fix some module refereces
  • Fix README example
  • More logging
  • Fix run_until_complete
  • Add travis configuration
  • Fix(?) Travis config
  • Fix context manager logic
  • Support PySide
  • Implement subprocess_exec

File Changes

(23)

(53)

Patch Links:

—
Reply to this email directly or view it on GitHub
#2.

—
Reply to this email directly or view it on GitHub
#2 (comment).

—
Reply to this email directly or view it on GitHub
#2 (comment).

—
Reply to this email directly or view it on GitHub
#2 (comment).

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 1, 2014

I've changed spaces to tabs, hopefully it'll be easier to review.

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 2, 2014

After testing the 'unix' branch on Windows and fixing correspondingly, I merged it into master. it still doesn't work for subprocess stuff on Unix, but should provide a good starting point.

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 4, 2014

@harvimt Have you had a time to look it over yet? I think the Windows side of things should be pretty good at this point, and my tests are finally running on Travis (but not successfully, due to it being Linux).

I'm working on the Unix adapter in a separate branch, I think I've sussed out how it's supposed to work at least.

@harvimt
Copy link
Owner

harvimt commented Jul 4, 2014

I've had some time to look at it.

Looks like you actually got easycallback to work.

easycallback was intended to be an externally usable decorator (even though
it has nothing to do with asyncio, it's a convenient utility for working
with signals/slots). In general you've added _ and __ to a lot of variables
that I wouldn't have, but those sorts of things aren't really worth
rejecting a patch over. Remember __ is not for private variables: it's for
avoiding naming conflicts with different member variables from parent or
child classes. (I'm pretty sure this is described here:
http://pyvideo.org/video/879/the-art-of-subclassing). The use of
get_debug/set_debug and self.__debug

The exception handler is a good idea, I've definitely had problems with
exceptions just getting sent to the void.

The _common module with just one function in it seems excessive, could be
moved to init or maybe more stuff could be moved to _common from
init (basically everthing that does stuff), basically everything after
line 12

I haven't actually used asyncio for TCP/IP stuff yet (haven't needed to),
I've been trying to get it to work for subprocess & thread stuff. Though I
have looked at the tutorials it all looks good.

It will probably be good to add tests to test the QThreadExecutor (probably
in a new module test_qthreadexecutor under tests, I think there are
doctests though, might just use those). I can probably handle that since
it's one of my motivated use cases. Also need tests for start_server() etc.
the TCP/IP stuff, I probably can't write those tests. I think good test
coverage and good logging coverage (thanks there again, I'd been pretty
lazy on that front) will be important in maintaining this going forward.

I don't actually have a machine that can run any of this stuff because it's
hard. I don't think there were even binary packages for PyQt5 and/or PySide
w/ Qt5 for Python 3.4 until recently, so I only had ArchLinux PySide with
Qt5 in a VM.

It'd be super nice if it would work in Qt 4 & 5 with PySide and PyQt, on
all 3 major platforms (Mac, Linux & Windows), but that may be an impossible
goal.

I've rearranged all my HDs since I got a new SSD, and some of that stuff is
in weird places.

This was always a side-project, with the hopes that it would maybe make
some of my other side projects involving Qt easier. (anything to CBZ
converter, mod manager for Bethesda games). My day job is web-dev at that's
been busy lately.

I'll try to get something working next week with like an actual test
sandbox. I'll approve the pull request once I can verify that the tests
that exist actually run.

README.rst should be a symlink to README. github needs the .rst to know
that it's rst and not markdown. By convention python projects have a README
(no file extension) that contains rst. This is hard to keep working when
checking out on Windows I think (I may very well have broken this at some
point, I'm still having a hard time reading the git logs)

setup.py should be updated. (no more depends tulip, I don't know if there's
a way to depend on either PyQt or PySide, I don't think so, at least some
sort of thing in the short description). I don't think the regex will work
as intended now that you've added your name to the author list. (will need
to be some sort of .split(', ') + list comprehension or .findall(...) type
thing)

After all that, we could probably call it v1.0, maybe even upload it to
PyPi.

Some sort of continuous integration would be nice, I started to play around
with travis before becoming distracted.

Alright, I'm gonna get another beer, I'll get back to you next week.

On Fri, Jul 4, 2014 at 8:13 AM, Arve Knudsen notifications@github.com
wrote:

@harvimt https://github.com/harvimt Have you had a time to look it over
yet? I think the Windows side of things should be pretty good at this
point, and my tests are finally running on Travis (but not successfully,
due to it being Linux). I'm working on the Unix adapter in a separate
branch, I think I've sussed out how it's supposed to work at least.

—
Reply to this email directly or view it on GitHub
#2 (comment).

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 5, 2014

The use of double leading underscores is completely intentional, I know that it leads to name mangling and not really "private" attributes. However, I consistently use double underscores for attributes that are meant to be private, for two reasons: 1. Avoid name clashes when subclassing (have had it happen in the past), 2. Make it absolutely clear that an attribute is an implementation detail and subject to change. Single leading underscores I typically use for "protected" attributes, that may be used throughout the class hierarchy and/or by "friends". Without being able to distinguish between the two, I can't be sure of what's safe to change/remove. The interface is kept as small as possible (which is the guiding principle), but nothing is really hidden from the client either. I've been following this scheme for years, and it's served me well.

As regards _common, it'll contain more code once the unix branch is merged into master.

There is at least an indirect test of QThreadExecutor, at the moment.

It works with PySide (Qt 4.8) and PyQt 5 on Linux, Windows and Mac for me. It's tested continuously on Travis with PySide. The main problem with that is I don't have a binary Linux installer for PySide, so it's built from source on Travis, which takes a long time.

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 5, 2014

Note that I now have Quamash working on Unix as well (tested on OS X currently), in the unix branch. Do you want me to merge it into master (after verifying it on Windows ofc.) before you review this pull request?

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 5, 2014

The unix branch is now verified as working on Linux, OS X and Windows :) I'm quite happy with the Unix implementation as well, since I was able to implement file/socket polling as reacting to QSocketNotifiers, meaning that there's no need for a background event loop as there is on Windows. Instead, file/socket polling is directly integrated with the Qt event loop, making it simplicity itself!

aknuds1
aknuds1 reviewed Jul 9, 2014
View reviewed changes
setup.py
packages=['quamash', ],
license=quamash.__license__,
depends=['tulip', 'PyQt', ],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

depends is apparently not a supported option to setup. It's possible to instead use setuptools and dependency_links.

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 9, 2014

I've fixed setup.py now, so that authors and email addresses are correctly specified.

@harvimt harvimt merged commit 5477ecf into harvimt:master Jul 11, 2014
@harvimt
Copy link
Owner

harvimt commented Jul 11, 2014

Cool, I fixed the doctests, and they pass.

I switched from distutils to setuptools (I really like being able to python
setup.py develop, also python setup.py wheel)

Cancelable's weren't being returned by the call* methods, that's fixed.

It'd be nice to get more tests really, but this seems pretty good for now.

I changed the demo app to something a little bit more interesting, it now
updates a progress bar either asynchronously (in both the same thread and
in a different thread)

Anyway, Quamash is a lot more useful thanks to you, but I'm glad I was at
least able to sketch an outline so you could fill in the middle.

On Wed, Jul 9, 2014 at 1:14 PM, Arve Knudsen notifications@github.com
wrote:

I've fixed setup.py now, so that authors and email addresses are correctly
specified.

—
Reply to this email directly or view it on GitHub
#2 (comment).

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 11, 2014

Thanks! I'll be sending you a PR for my UNIX branch as well, for some multi platform goodness.

@harvimt
Copy link
Owner

harvimt commented Jul 11, 2014

ah. to clarify, I pulled the unix branch.

But you can send another one.

On Thu, Jul 10, 2014 at 11:39 PM, Arve Knudsen notifications@github.com
wrote:

Thanks! I'll be sending you a PR for my UNIX branch as well, for some
multi platform goodness.

—
Reply to this email directly or view it on GitHub
#2 (comment).

@aknuds1
Copy link
Collaborator Author

aknuds1 commented Jul 11, 2014

Aha you did, good thinking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aknuds1 @harvimt