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.

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

Fix compatibility with Python 2 #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lrq3000
Copy link

@lrq3000 lrq3000 commented Jan 27, 2018

This PR fixes a compatibility issue with Python 2: decorating with just @widgets.register will transform the decorated class into a function, whereas using @widgets.register() will correctly return a class. This fixes the following error:

TypeError: Error when calling the metaclass bases function() argument 1 must be code, not str

With this fix I could successfully run the following simple example:

import ipyvolume
ipyvolume.examples.ball(rmax=3, rmin=2.5, shape=64, lighting=True)

BTW it would be nice I think to add the mybinder examples in the doc directly, or even the readme, so that new users can easily test out the widget for themselves :-) Otherwise in the doc there is no other example that does not require a 3rd-party library (bokeh etc).

@coveralls
Copy link

coveralls commented Jan 27, 2018

Coverage Status

Coverage remained the same at 61.763% when pulling 2f1f89a on lrq3000:py2fix into 6e1f088 on maartenbreddels:master.

@lrq3000
Copy link
Author

lrq3000 commented Jan 27, 2018

Also for reference (about the decorator):

@maartenbreddels
Copy link
Collaborator

Hi there, thanks for your contribution!
I cannot reproduce this actually.
The decorator might be a bit confusing, since going from ipywidget 6 to 7 it does not take a name anymore, but a widget instead. If you installed ipyvolume with ipywidget 6 this may happen actually!.
A binder link in the docs would be great, I saw some auto generated once, I'll look into it.

@lrq3000
Copy link
Author

lrq3000 commented Feb 8, 2018

@maartenbreddels mmm that's weird, did you try with Python >= 2.7.13 ? Because it's known to work with Python >= 3 since the class management changed, so this issue affects only Python 2. I have ipywidget v7.1.1, I had to update else the widget would not even load with the time old issue of "javascript can't be loaded" or something like that.

@maartenbreddels
Copy link
Collaborator

it's 2.7.14, on OSX, using miniconda, and ipyvolume works with that. I'm almost sure it is ipywidget 6 you are using, maybe you have it installed in ~/.local?
can you check the output of python -c "import ipywidgets as w; print(w.__version__, w.__file__)" ?

@lrq3000
Copy link
Author

lrq3000 commented Feb 13, 2018

Hey @maartenbreddels , thank you for your suggestion, here is the output:

('7.1.1', 'C:\\Anaconda2\\lib\\site-packages\\ipywidgets\\__init__.pyc')

The path is where I initially applied my patch to make ipyvolume works. Also my Python version is 2.7.12, maybe there was an update between .12 and .14 that renders Py2 compatible with such decorator?

@maartenbreddels
Copy link
Collaborator

Ok, I need to check this on windows, I only have OSX and Linux now, but I'll try to boot up a virtual machine soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants