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

custom mimetype support - add direct JSON #1173

Open
DougBurke opened this issue May 16, 2020 · 14 comments
Open

custom mimetype support - add direct JSON #1173

DougBurke opened this issue May 16, 2020 · 14 comments

Comments

@DougBurke
Copy link

Can we add some way to pass JSON through displayDataToJson for the new custom mimetype? Rarndom thoughts are

a) add a separate mimetype - e.g. MimeCustomJSON Text - which can then be used in disolayDataToJson in a similar way to how MimeJson/MmeVegaite/MimeVega is handled

b) or change the MimeCustom type to treat the value in displayDataToJson directly as an Aeson Value

The bit I'd like to be able to handle is to pass a custom mimetype with a Value, not a String, in the followig.

-- from https://github.com/gibiansky/IHaskell/blob/master/ipython-kernel/src/IHaskell/IPython/Types.hs

displayDataToJson :: DisplayData -> (Text, Value)
displayDataToJson (DisplayData MimeJson dataStr) =
    pack (show MimeJson) .= fromMaybe (String "") (decodeStrict (Text.encodeUtf8 dataStr) :: Maybe Value)
displayDataToJson (DisplayData MimeVegalite dataStr) =
    pack (show MimeVegalite) .= fromMaybe (String "") (decodeStrict (Text.encodeUtf8 dataStr)
:: Maybe Value)
displayDataToJson (DisplayData MimeVega dataStr) =
    pack (show MimeVega) .= fromMaybe (String "") (decodeStrict (Text.encodeUtf8 dataStr) :: Maybe Value)
displayDataToJson (DisplayData mimeType dataStr) =
  pack (show mimeType) .= String dataStr

@vaibhavsagar
Copy link
Member

What is the problem you're trying to solve here? Could you encode the Value as a Text and use the existing data types? If it turns out to be necessary to add a new mimetype we can do that but I'm wary because each time it breaks backwards compatibility and complicates the interface.

@DougBurke
Copy link
Author

DougBurke commented May 17, 2020 via email

@vaibhavsagar
Copy link
Member

Okay, I'd accept a PR adding this additional mimetype.

@vaibhavsagar
Copy link
Member

I think this might be tricky to implement at least in part because of this line:

https://github.com/gibiansky/IHaskell/blob/d7dc460a421abaa41e04fe150e264bc2bab5cbad/ipython-kernel/src/IHaskell/IPython/Types.hs#L828

How would read know that something is a MimeCustomJSON instead of a regular MimeCustom?

@DougBurke
Copy link
Author

#1176 is my initial attempt at this, but without resolving the readsPrec issue

@DougBurke
Copy link
Author

I now am wondering if it would be better to just add an extra parameter to CustomMimetype to allow you to decide how the Text payload is converted to JSON - e.g. just Aeson.String or the fromMaybe (String "") (decodeStrict (Text.encodeUtf8 dataStr) :: Maybe Value) displayDataToJson (DisplayData mimeType dataStr) approach

@DougBurke
Copy link
Author

Darn it. I was playing around with

data MimeType = PlainText
              | MimeHtml
              | MimeBmp Width Height
              | MimePng Width Height
              | MimeJpg Width Height
              | MimeGif Width Height
              | MimeSvg
              | MimeLatex
              | MimeMarkdown
              | MimeJavascript
              | MimeJson
              | MimeVega
              | MimeVegalite
              | MimeVdom
              | MimeCustom Text (Text -> Value)
  deriving (Eq, Typeable, Generic)

but the need instances give me pause (due to Text -> Value). We could instead have a simpler version, that lets you chose between String and fromMaybe (String "") (decodeStrict (Text.encodeUtf8 dataStr) :: Maybe Value)

@DougBurke
Copy link
Author

I've added a second commit which uses a different design (moves the choice for the encoding to the CustumMimetype type).

@vaibhavsagar
Copy link
Member

It still doesn't look like this new approach fixes the problem with readsPrec. I'm actually now leaning towards just keeping the wasteful old behaviour, it's far from ideal but I can't see another way that would also work sensibly in that case.

@DougBurke
Copy link
Author

Do you want me to switch #1176 back to the original version?

@vaibhavsagar
Copy link
Member

I don't think it makes a difference, the original version had the same issue.

@DougBurke
Copy link
Author

Given the way the Read instance works for other types (e.g. hardcoding width / height) I'm personally don't think it is a serious issue (and technically I think the second version is "better" for Read, but not massively so).

@DougBurke
Copy link
Author

In coming back to this, we have

MimeCustom Text

which is - I think - processed by this step

displayDataToJson (DisplayData mimeType dataStr) =
  pack (show mimeType) .= String dataStr

What I want is for it to not encode the data as a String but as JSON, that is something like

displayDataToJson (DisplayData (MimeCustom mimeType) dataStr) =
    mimeType .= textToJson dataStr

-- pulled out as it's used in several mime types
textToJson :: Text -> Value
textToJson dataStr = fromMaybe (String "") (decodeStrict (Text.encodeUtf8 dataStr) :: Maybe Value)

Last time I tried I don't think I was able to do this as a user of IHaskell.Display - that is create the JS code and convert it to a String value for use with a custom mimetype, but I should go back and check this.

@DougBurke
Copy link
Author

So, I've created a notebook showing that I can't seem to use the custom mimetype to replicate the vega-lite mimetype because of this. You can find it at https://gist.github.com/DougBurke/11009ad97320e352c21a0d7558bb0b8b but the gist "auto display" doesn't seem to want to work for me.

In writing this I did note that the vega-lite mimetype has been bumped up to v4 so that means I personally am not affected by this for the moment (although the vega/vega-lite schema keeps changing so it could be an issue in the future). So I'm probably/hopefully not going to be spending much time on this in the near future.

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