scrapbook icon indicating copy to clipboard operation
scrapbook copied to clipboard

Logic around interpreting(…encoder=None, display=['any/mimetype'])

Open mpacer opened this issue 5 years ago • 1 comments

This is arising from some confusion I'm having around the current glue displaying api. I am likely to create a few issues like this… I'm guessing most of them will be closed without action.

https://github.com/nteract/scrapbook/blob/e06182f034c9ee231e491d66047c191e1d0d0f07/scrapbook/api.py#L82-L84

The comment seems to say one thing is intended and then the behaviour is otherwise, where if the display is not None, and encoder is undefined, then it is assumed to not be display.

However, if I'm defining a display type (so display would be not None but also would not evaluate to False), it would seem to be reasonable to encode it as a display as well (without needing to declare it as the display encoder).

So should the logic earlier include something like

if encoder is None and display:
    encoder = "display"

?

mpacer avatar Mar 21 '19 04:03 mpacer

Ahh that comment should probably be up in https://github.com/nteract/scrapbook/blob/e06182f034c9ee231e491d66047c191e1d0d0f07/scrapbook/api.py#L69-L73 as I was thinking that there maybe should be an encoder that can detect ipython display objects automatically to set the encoder to display.

Doing if encoder is None and display: below the encoder selector where will never trigger because the lines above will always assign json if nothing is found: https://github.com/nteract/scrapbook/blob/e06182f034c9ee231e491d66047c191e1d0d0f07/scrapbook/api.py#L78-L80 . Are you thinking we should add the proposed line above the encoder selector block that's there today? I think that'd be a reasonable change.

I'm addressing many of the TODO's in the reference data PR I'm still working on, but hadn't tackled making a display encoder, maybe this solution you proposed would be more elegant and easy to read. Want to PR it?

MSeal avatar Mar 21 '19 15:03 MSeal