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

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

Open
mpacer opened this issue Mar 21, 2019 · 1 comment
Open

Comments

@mpacer
Copy link
Member

mpacer commented Mar 21, 2019

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.

# TODO: default to 'display' encoder when encoder is None and object is a display object type?
if display is None:
display = encoder == "display"

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"

?

@MSeal
Copy link
Member

MSeal commented Mar 21, 2019

Ahh that comment should probably be up in

# TODO: Implement the cool stuff. Remote storage indicators?!? Maybe remote media type?!?
# TODO: Make this more modular
# TODO: Use translators to determine best storage type
# ...
if not encoder:

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:

else:
# This may be more complex in the future
encoder = "json"
. 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?

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

No branches or pull requests

2 participants