search

pklaus / ds1054z

Python package for the Rigol DS1054Z Oscilloscope
https://ds1054z.readthedocs.org/
178 stars 40 forks source link

in discovery.py print/raise error including "requires zeroconf package" when zeroconf is not installed #11

Open nmz787 opened 6 years ago

nmz787 commented 6 years ago

I just tried import ds1054z.discovery and found it failed to import. Got confused by the main page instructions pip install ds1054z[savescreen,discovery] which don't make sense to me because of the square brackets (I haven't seen that in pip syntax documentation before). Finally I checked the github directory tree and saw the comment at the top of discovery.py saying I needed zeroconf if I got the import error. So why not print an additional message for less skilled users than myself?

pklaus commented 6 years ago

Thanks for reporting the issue. Please check out https://stackoverflow.com/a/46775606/183995 for an explanation of the pip install syntax used. If I find the time, I'll improve the output in case the package is missing. Feel free to file a pull request. Thanks!

nmz787 commented 6 years ago

This issue has nothing to do with pip install. Seems a sufficient enhancement would be a try/except around the import and print a warning to install zeroconf if the import hits the except statement.

On Mon, Sep 3, 2018, 6:27 AM Philipp Klaus notifications@github.com wrote:

Thanks for reporting the issue. Please check out https://stackoverflow.com/a/46775606/183995 for an explanation of the pip install syntax used. If I find the time, I'll improve the output in case the package is missing. Feel free to file a pull request. Thanks!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pklaus/ds1054z/issues/11#issuecomment-418115299, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE7RkgoNIW9bAzwdkYuRR6Xs1Y9mja5ks5uXS5DgaJpZM4WStTV .

pklaus commented 6 years ago

This issue has nothing to do with pip install.

This is what you think. Just try running the command with the square brackets as proposed and you'll see that the zeroconf package is getting installed alongside due to line 45 in setup.py, see setuptools' documentation on that feature.

But yes, you're right: there should be an appropriate warning if the package isn't around while trying to import it.

nmz787 commented 6 years ago

Hmm, interesting, I didn't know this was a feature of setuptools but unfortunately the top google hit for me searching pip install square brackets doesn't explicitly call out that this syntax given to pip will pass-through to setuptools and also force the installation of the feature's requirements. For a relatively seasoned Pythonista who is still a newb when it comes to packaging one's tools and libraries (me), this just flew right through my radar.

Since you aren't here to teach the world about pip and setuptools I think simply raising the real ImportError or whatever it is, seems the most appropriate/Pythonic and least work for you.