search

bkjones / pyrabbit

A Python module to communicate w/ the RabbitMQ HTTP Management API
http://pyrabbit.readthedocs.org
BSD 3-Clause "New" or "Revised" License
97 stars 86 forks source link

hi there bkjones, a pull request for you #2

Closed dkuebric closed 12 years ago

dkuebric commented 12 years ago

We've got two commits here that solve different issues:

  1. There's an unnecessary import of api in __init__.py, which causes problems with installing the dependencies (httplib2) because it requires them before it can find out they're specified! Removing the import seemed like the right fix.
  2. Patch from @cce to fix: If you don't have permission to check is_alive, it'll crash.
bkjones commented 12 years ago

Gonna take some time tonight and flesh out tests for the last pull request, and this one, and for another bug I found myself. Fixes for these issues will be pushed sometime in the next several days, if not tonight :) Thanks a million for your efforts.

bkjones commented 12 years ago

I believe the two immediate issues raised here are fixed. First, is_alive should've been decorated with @needs_admin_privs. I added that, as well as tests for that condition. I realize this doesn't address the issue in the HTTPError where 'content' is an empty string. I'd like to spend more time with httplib2 on that (well, I don't want to per se, but...) ;-)

As for the unnecessary api import, I actually meant to change that some time ago to import the Client - not the api, so I did that. Now you can do 'from pyrabbit import Client', since that's the only object intended for use by users of pyrabbit anyway. To get around the dependency issue at installation time, I changed setup.py so it imports the version attribute it needs explicitly instead of just doing 'import pyrabbit'. Please let me know if this doesn't work out for some reason.

Thanks again, guys! I'm gonna close this for the moment, but feel free to reopen or open a new issue/pull request. I'd also love to see what you're doing with pyrabbit, so drop me a link!

Oh, here's the commit -> https://github.com/bkjones/pyrabbit/commit/bcf2f8c7576ef08153e084dc93ca53a7b02258df

dkuebric commented 12 years ago

Sounds like you found better treatment for the is_alive case, cool!

In terms of httplib2, I think in order to import a part of a module, it must still be parsed and evaluated as far as necessary to get the desired attribute. In this case, that means that top-level imports are still being evaluated, and their top-level imports, etc. So, I'm still seeing the problem even with the new import of Client only.

(venv27)dan@dev:~/pyrabbit$ python setup.py egg_info Traceback (most recent call last): File "setup.py", line 2, in from pyrabbit import version File "/home/dan/pyrabbit/pyrabbit/init.py", line 1, in from .api import Client File "/home/dan/pyrabbit/pyrabbit/api.py", line 7, in from . import http File "/home/dan/pyrabbit/pyrabbit/http.py", line 2, in import httplib2 ImportError: No module named httplib2

You should be able to reproduce this by removing httplib2 from your environment and trying to install/egg_info it.

On Fri, Nov 11, 2011 at 11:13 PM, Brian K. Jones < reply@reply.github.com

wrote:

I believe the two immediate issues raised here are fixed. First, is_alive should've been decorated with @needs_admin_privs. I added that, as well as tests for that condition. I realize this doesn't address the issue in the HTTPError where 'content' is an empty string. I'd like to spend more time with httplib2 on that (well, I don't want to per se, but...) ;-)

As for the unnecessary api import, I actually meant to change that some time ago to import the Client - not the api, so I did that. Now you can do 'from pyrabbit import Client', since that's the only object intended for use by users of pyrabbit anyway. To get around the dependency issue at installation time, I changed setup.py so it imports the version attribute it needs explicitly instead of just doing 'import pyrabbit'. Please let me know if this doesn't work out for some reason.

Thanks again, guys! I'm gonna close this for the moment, but feel free to reopen or open a new issue/pull request. I'd also love to see what you're doing with pyrabbit, so drop me a link!

Oh, here's the commit -> https://github.com/bkjones/pyrabbit/commit/bcf2f8c7576ef08153e084dc93ca53a7b02258df

Reply to this email directly or view it on GitHub: https://github.com/bkjones/pyrabbit/pull/2#issuecomment-2716011

bkjones commented 12 years ago

Of course you're right. I just tested using 'install', but I guess I didn't perform that test in a clean environment before pushing my changes. I should do fewer pushes late at night. I'll get this fixed. Apologies for the mixup.

bkjones commented 12 years ago

Ok, just did a test in a clean environment after applying a fix for the dependency issue in setup.py. I have a good idea of how to fix this, but it requires a bit more tinkering on my part and I'd like to get something installable out sooner rather than later. Please let me know if anything goes bump for you - works for me at this point, and I think the fix should work, even though I wouldn't consider it optimal (version is now hard-coded in setup.py) :-/

dkuebric commented 12 years ago

Works great, thanks!

On Mon, Nov 14, 2011 at 3:28 PM, Brian K. Jones < reply@reply.github.com

wrote:

Ok, just did a test in a clean environment after applying a fix for the dependency issue in setup.py. I have a good idea of how to fix this, but it requires a bit more tinkering on my part and I'd like to get something installable out sooner rather than later. Please let me know if anything goes bump for you - works for me at this point, and I think the fix should work, even though I wouldn't consider it optimal (version is now hard-coded in setup.py) :-/

Reply to this email directly or view it on GitHub: https://github.com/bkjones/pyrabbit/pull/2#issuecomment-2736066