Skip to content

Simple bug-fix #149

Closed
wants to merge 5 commits into from

3 participants

@rkern
rkern commented Sep 17, 2010

The result_display hook was removed from the list of available hooks. My beautiful pretty extension was broken. It made me sad.

@ellisonbg
IPython member

Was this sufficient to fix the the pretty extension?

@rkern
rkern commented Sep 17, 2010

No, it just allows the hook to be registered as the extension is written to do. No one does anything with that hook in the newkernel branch. This still makes me sad. What exactly needs to be decided about the hooks (per the TODO comment in displayhook.py)?

@ellisonbg
IPython member

The big picture with the hook is that we wan to move away from global things towards using good encapsulation/OO design. Also hook became a bit of cat chasing its tail. The core calls the hooks calls the core calls the... Thus, as we are refactoring the core, we are removing hooks wherever possibly. For result_display this happened when we refactored the display hook this summer. To get the functionality back, we simply need to create a simple API on the display hook object in the core and then have the extension call that API. In the long run, we want to come up with a nice generic way of handling hook type things on our classes, but we don't need to get this worked out before we fix result_display. I just added this to my todo list on the Google doc...

fperez added some commits Sep 18, 2010
@fperez fperez Add function signature info to calltips.
Also removed custom ObjectInfo namedtuple according to code review
(left as a dict for now, we can make it a list later if really needed).

Added the start of some testing for the object inspector and updated
the messaging spec.
f875d43
@fperez fperez Remove leftover files from bzr. 742d5ef
@rkern
rkern commented Sep 20, 2010

Can that API be a CommandChainDispatcher on the DisplayHook? Or is the design of CommandChainDispatcher part of the problem you have with hook mechanism? I fear that you will either end up reinventing CommandChainDispatcher or coming up with slightly different mechanisms for each of the extension points.

@rkern
rkern commented Sep 20, 2010

I switched branches around, so this pull request doesn't make sense anymore. But my question still stands: What do you want that API to look like?

@ellisonbg
IPython member

There are a couple of issues that are driving the reconsideration of the hooks:

  • The global nature of the hooks are the biggest issue. We want to move to a model where the different components of the IPython core have their own extension points that are local to those classes and encapusulated therin.
  • The notion of an IPython extension point goes far beyond the current "hooks" model. As we have been refactoring the core, we are introducing other non-hook APIs that are meant to be part of the developer API for the core.
  • I would say that at the current time, it is not yet clear what all the extension APIs should look like and we haven't spent much time thinking about it.
  • It is very possible that the CommandChainDispatcher would be one way we would offer extension APIs. We would need to think about how to fit that into the more encapsulated context of a class. I have even thought about using traits/traitlets to declare extension points.

I should mention that the other part of this picture is that there are still some aspects of the display hook and payload system that we need to work out. We are currently using our new payload system to bring back things like the result of page calls and inline graphics. Some of this stuff may end up being moved to the display hook.

While there are lots of larger issues still being played out, I don't see any reason we can't add back some sort of simple extension API for the display hook that the pretty printer extension can use.

@rkern
rkern commented Sep 21, 2010

Alternately, we can use pretty as the default implementation and use its extension API to allow people to add individual formatters for individual types. I suspect that's what most people want to extend.

@fperez
IPython member
fperez commented Sep 21, 2010

Robert, I had my gmail filters set a little too aggressively and hadn't seen this, sorry; Brian just pointed me to it.

I saw you closed this one and opened an updated pull request, do you prefer to continue the discussion there?

@rkern
rkern commented Sep 22, 2010

We can discuss here since it's pretty clear that merging in the changes that just add back the result_display hook are not what we want. I do not have another pull request open for this.

@rkern
rkern commented Sep 22, 2010

Or ipython-dev, really.

@ellisonbg
IPython member

Fernando and I talked about the display hook logic a bit yesterday. Let's continue this discussion on ipython-dev though.

@fperez
IPython member
fperez commented Sep 22, 2010

OK, I'm a bit swamped today but I'll be happy to continue tomorrow on ipython-dev until we find an approach we're happy with for the long run.

@minrk minrk pushed a commit to minrk/ipython that referenced this pull request Jul 1, 2013
@Carreau Carreau remove pypandoc
Closes #149 (replacement of it since refactor)
1f16ad5
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.