IPEP 10: kernel side filtering of display formats #2981

Merged
merged 8 commits into from Apr 12, 2013

Projects

None yet

2 participants

Owner
minrk commented Feb 25, 2013

Draft implementation of IPEP 10

This adds active_types list to DisplayFormatter as a global default for include,
rather than always having default of everything.

There is no change whatsoever for the default behavior of any zmq-based IPython kernels (notebook, qtconsole, etc.).
The only actual effect under default conditions is to eliminate the cost of registering unused formatters in terminal IPython.

Owner
minrk commented Mar 29, 2013

I realize that we already have enabled flag on each display formatter, so perhaps this implementation should be a bit different, and maybe the active_types attribute should be removed. The only reason I see to keep it is so that the list can be set via a config file, but it should probably not be where the actual state is stored.

Owner
minrk commented Apr 3, 2013

Right now, active_types is basically a property exposing formatter.enabled, but in a write-only way. I don't know how to make a trait behave like a property on read. As it is, the active_types configurable is simply a proxy to the formatter.enabled attributes, and the internal implementations remain unchanged.

minrk added some commits Apr 3, 2013
@minrk minrk implement active_types via formatter.enabled attributes
it would be ideal if this could just be a property,
but I don't know how to make a trait a property.
e3d95d5
@minrk minrk simplify include/exclude conditions 001a008
@minrk minrk use items, not keys, duh 72c522d
@ellisonbg ellisonbg commented on an outdated diff Apr 8, 2013
IPython/core/formatters.py
+ warnings.warn("""DisplayFormatter.plain_text_only is deprecated.
+
+ Use DisplayFormatter.active_types = ['text/plain']
+ for the same effect.
+ """, DeprecationWarning)
+ if new:
+ self.active_types = ['text/plain']
+ else:
+ self.active_types = self.format_types
+
+ active_types = List(Unicode, config=True,
+ help="""List of currently active mime-types""")
+ def _active_types_default(self):
+ return self.format_types
+
+ def _active_types_changed(self, name, old, new):
ellisonbg
ellisonbg Apr 8, 2013 Owner

If I understand correctly, this method is where the active_types actually affects the display logic by setting enabled to True of False. Is that right?

Owner

This code is clean and follows the IPEP 10 proposal closely. I like it. My only hesitation is that I don't want to encourage people to start turning various display formats off. How could we discourage that while still holding to this approach?

Owner

Is active_types actually used as a default for include? Or is this handled by the logic in active_types_changed?

Owner
minrk commented Apr 9, 2013

It's not a default for include. That was my initial implementation, but it seemed redundant with the already existing enabled flags, now active_types is a simple proxy for the enabled attribute of each.

If you do want to discourage this behavior, one thing we could do is make it not configurable, and just a plain property. There are aspects of the implementation that would be cleaner, but I'm not quite convinced that's the right approach.

Owner
minrk commented Apr 9, 2013

I've updated IPEP 10 to describe the current state of this PR.

Owner

OK, thanks for updating IPEP 10. I think this is a clean design and implementation. I am +1 on merging this.

Owner
minrk commented Apr 10, 2013

What do you think about making active_types a simple property instead of a configurable?

Owner

I am 50/50 on making active_types a property. I am fine merging this as is if you want.

Owner
minrk commented Apr 11, 2013

A property implementation is definitely cleaner, but it eliminates the configurability - do you think we want it to be a configurable?

@ellisonbg ellisonbg merged commit 2dcf5c5 into ipython:master Apr 12, 2013

1 check passed

default The Travis build passed
Details
@minrk minrk deleted the minrk:format_types branch Mar 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment