Is it okay to implement Kino.Render for Map?

mruoss

Hey there

The title basically says it all. Here’s what I would like to do in kino_k8s. I’d like to render maps that are (look like) Kubernetes resources as YAML markdown. And maybe later render maps that are lists of Kubernetes resources as table. Unfortunately, these are regular maps, not structs. I fear conflicts if multiple libs do the following. Or is this safe to do?

defimpl Kino.Render, for: Map do
  def to_livebook(resource)
      when is_map_key(resource, "apiVersion") and
             is_map_key(resource, "kind") and
             not is_map_key(resource, "items") do
    """
    ```yaml
    #{Ymlr.document!(resource)}
    ```
    """
    |> Kino.Markdown.new()
    |> Kino.Render.to_livebook()
  end

  def to_livebook(resource), do: Kino.Output.inspect(resource)
end

11 comments

/livebook #kino

0 934 11

2023-11-06 08:41:08 UTC

Showing Posts 1 to 10

Show Best Posts
Show All (oldest first)
Show All (newest first)

nulltree

Reminds me of Rust.

Rust’s orphan rule requires that either the trait or the type for which you are implementing the trait must be defined in the same crate as the impl.

There, a typical solution would be to wrap the foreign type in an own. Translated to Elixir, maybe you could parse your map to a struct and then implement Kino.Render for that?

Am aware I’m not answering your actual question, though.

Post #1

mruoss

I think I see what you mean. But that is not really a possibility. It would have to be done in the k8s library (because the point of implementing Kino.Render in kino_k8s is to render what k8s returns as YAML. And currently, k8s returns resources as maps.

Converting maps into structs in k8s would be super nasty (tons of “unsafe” calls to String.to_atom() etc.) because a Kubernetes resource could be of arbitrary shape containing arbitrary data (think of CRDs).

Post #2

nulltree

I was thinking along the lines of having a step that stores (‘parsing’ was imprecise) the map in a struct shaped like this and then defimpling on MyStruct.

defmodule MyStruct do
  defstruct : k8s_result: %{}
end

But that may not fit your case ofc.

Post #3

mruoss

Oh okay yes, that is more feasible. Gotta think about that…

Post #4

mruoss

Yes, I could add something like this as the last instruction of the generated code: struct!(MyStruct, k8s_result: resource). But in the smart cell, I can also just return Kino.Markdown directly (which is what I’m doing now).

But if somebody plays with the k8s library, they would still get a “normal” map from k8s…

Post #5

nulltree

That’s how I understood the requirements so far, and yes, this should fit the bill.

Ofc, the k8s library could also return this kind of ‘type-wrapper’ struct, but I am not yet versed enough in Elixir to understand whether that would be considered idiomatic.

Post #6

mruoss

If you implement Kino.Render, the implementation is used to render the result of any cell. Not only smart cells.

Ofc, the k8s library could also return this kind of ‘type-wrapper’ struct,

True. Would be hell of a change of its API, though

Post #7

nulltree

Yeah, tbh I never worked with Kino or smart cells so far and am a bit out of my depth regarding best practices here, just wanted to introduce the idea of a ‘newtype’ / ‘type wrapper’ as I could see multiple implementations conflicting. So far wrapping the map in the struct and then render the structs implementation seems reasonable for my limited understanding, but am sure others with more experience can chime in.

True.

Post #8

mruoss

In any case thanks your input.

Post #9

jonatanklosko

Creator of Livebook

Hey, I don’t think we should be implementing Kino.Render for any built-in data types outside of kino, because doing that would lead to conflicts sooner or later. Unless k8s returns a struct, I think the user should call |> KinoK8s.resource() or similar to convert map to kino markdown (and the smart cell can do that for them).

Post #10

Load more posts (1 remaining)