Skip to content

Added repr-based responses and corresponding options#84

Merged
takluyver merged 4 commits intomasterfrom
repr-display
Apr 19, 2015
Merged

Added repr-based responses and corresponding options#84
takluyver merged 4 commits intomasterfrom
repr-display

Conversation

@flying-sheep
Copy link
Member

fixes #59

R/options.R Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why wouldn't we try all of them? It could make sense to have e.g. a png representation of something other than a plot.

Copy link
Contributor

Choose a reason for hiding this comment

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

ipython also has a knob to dis/enable some of the formats. It's used by the mpl inline backend to set the send back image formats.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway: the way the system there works, it really tries all of the enabled ones, so even for plots, the repr_markdownwould be tried (and return nothing)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have been clearer: I agree with having an option to configure this, but I'd put all the formats we know IPython can display in there by default - I think that just means adding image/png and image/svg+xml to this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

and many more. i’ll add them.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you think i should do the same for jupyter.plot_mimetypes?

how does ipython handle this?

Copy link
Contributor

Choose a reason for hiding this comment

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

IPython makes no difference between plotting and displaying anything else, the "knob" to influence the plotting formats is actually just is a convenience function, which (de-)activates the image formatter: https://github.com/ipython/ipython/blob/master/IPython/core/pylabtools.py#L172

@jankatins
Copy link
Contributor

Whatever comes out of these discussion: I think it would be good to add a blogpost like document to the default Jupyter documentations where this system is explained for "kernel developers", so that other can use that to model their own system after it...

I can build one after this text here: IRkernel/IRdisplay#3 (comment) (second part)

[Edit: added a issue here. https://github.com/jupyter/jupyter_client/issues/2]

@flying-sheep
Copy link
Member Author

since the discussion is hidden now:

the default for results is now everything.

do you think i should do the same for jupyter.plot_mimetypes?

i think it only makes sense to list those which actually have a repr_*.recordedplot method (excluding repr_text.recordedplot which i only wrote so that print(r.plot.instance) doesn’t do something silly)

@jankatins
Copy link
Contributor

I think the problem here is that if the plotting option is just "setting an option like "jupyter.plot_mimetypes" to a list", we miss out any validation/"translation" to the normal display formatter like the ipython equivalent does. Or is there a way to validate options when it is set via options(...)?

@flying-sheep
Copy link
Member Author

we could use the settings package (section “Using the settings package as options manager for your package”):

# not exported
MYPKGOPTIONS <- options_manager(a = 1, b = 2)

#' <documentation of options here>
#' @export
pkg_options <- function(...) {
    # protect against the use of reserved words.
    stop_if_reserved(...)
    # validate stuff
    opts <- list(...)
    if (!is.null(names(opts))) for (opt in names(opts)) switch(opt,
        a = stopifnot(condition),
        b = stopifnot(condition))
    # set or get
    MYPKGOPTIONS(...)
}

@flying-sheep
Copy link
Member Author

so, any objections to pulling this in?

@takluyver
Copy link
Member

I think this looks good.

takluyver added a commit that referenced this pull request Apr 19, 2015
Added repr-based responses and corresponding options
@takluyver takluyver merged commit f049abe into master Apr 19, 2015
@takluyver takluyver deleted the repr-display branch April 19, 2015 16:57
@asmeurer asmeurer mentioned this pull request Apr 24, 2015
10 tasks
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.

add setting for using HTML print function

3 participants