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

Refactor of IPython extension and rendering #308

Merged
merged 33 commits into from
Dec 1, 2015
Merged

Conversation

philippjfr
Copy link
Member

This PR will refactor the OutputMagic and display hooks such that they are backend independent and move some of the display machinery onto the Renderers. This will make the IPython extension cleaner and make it much easier to add new output formats and renders, and allow for static HTML exporting.

The PR is far from ready for merging.

@jlstevens
Copy link
Contributor

I've skimmed through the PR and it looks pretty good to me. Nice to see deletion from the IPython extension without a corresponding increase elsewhere. The approach of using renderer instance to keep track of the options (i.e as parameters) is very nice.

What in particular isn't ready yet? Is there anything you would like me to look at in more detail?

@philippjfr philippjfr force-pushed the output_refactor branch 4 times, most recently from a5408ef to 85f7e4a Compare November 30, 2015 20:24
@@ -12,44 +12,6 @@

archive = FileArchive()


Copy link
Contributor

Choose a reason for hiding this comment

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

Moving these two functions to util does make sense.

@jlstevens
Copy link
Contributor

Just a few comments (I'll extend this list as I go along):

  1. render_object should just be called render.
  2. collate_object should just be called collate (the function as opposed to the methods)
  3. The get_widget method seems to have the 'auto' option filtered out before it ever sees it. The 'auto' option makes sense for DynamicMap where the widget is decided based on the object definition (i.e open or closed).
  4. There is a bit of duplication between what list_formats does and what is happening in update_options. I suggest extending list_formats to be useful in both cases.

Other than these points and a few other comments I've made in the commits, I am very happy with this PR and ready to merge it!

@classmethod
def validate(cls, options):
"""
Validates a dictionary of options set on the backend.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the docstring should be something like "Validate an options dictionary for the renderer". The reason is that I would say that 'renderer' is a holoviews term (subclass of Renderer) whereas the 'backend' is the library or 3rd party tool used by the renderer. Very closely related, I agree, but not quite the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why not make validate accept **kwargs instead of a dictionary of options? Ends up being the same thing but maybe a nicer interface for validation. Just something to consider.

@jlstevens
Copy link
Contributor

Ok, I'm happy with this refactor. It really cleans things up and removing code from the ipython extension is always a good thing.

jlstevens added a commit that referenced this pull request Dec 1, 2015
Refactor of IPython extension and rendering
@jlstevens jlstevens merged commit f6cef64 into master Dec 1, 2015
@philippjfr philippjfr deleted the output_refactor branch December 1, 2015 03:08
@jlstevens
Copy link
Contributor

It is now merged and working. There is one more thing I would like to fix before 1.4 - currently the constructor of DynamicMap makes sure one item is in .data.

For generators, that means using next(..) and consuming one step of the generator. This should be moved out into either the display hooks or (probably even better) the renderer.

I'm happy to implement this change if Philipp tells me where he thinks this initialization should go. I can definitely remove this initialization from the DynamicMap constructor once it is done.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants