livebook-dev / kino_vega_lite

Vega-Lite (graphics) integration for Livebook
Apache License 2.0
26 stars 8 forks source link

Fix chart cell to handle empty data sets #20

Closed rliebling closed 2 years ago

rliebling commented 2 years ago

If you create a Chart smart cell while some of your bindings are for datasets with no results then the Chart cell fails to work. That is, you cannot choose any dataset in the "Data" input field. The problem is that the private function infer_types encounters an error because for the :rows case because it assumes there is at least one row.

The solution here is to explicitly check for whether the dataset has data. This does mean that a result set with no data won't be offered as a choice in the Chart smart cell. I could see instead having infer_types() return a list matching the length of the columns, but with either nil or "nominal" for each value so that the dataset would be allowed when empty.

josevalim commented 2 years ago

Hi @rliebling! The third element is not guaranteed to be a list, so perhaps the alternative fix is the way to go? Thank you!

rliebling commented 2 years ago

@josevalim Thanks for pointing that out! I updated the code, and the description of the PR. This feels clean, but I realized after that maybe one would still want to choose the empty dataset or at least you might get bug reports if they are not offered as choices. I'm fine changing to allow that, if you prefer. Just please let me know what infer_types should return for an empty dataset.

josevalim commented 2 years ago

@rliebling ah, that's another great idea. We could show empty datasets but we rule out any dataset without columns, and that will remove silly false positives such as empty lists (or empty maps).

jonatanklosko commented 2 years ago

Just please let me know what infer_types should return for an empty dataset.

A list of nils should work fine, as in Enum.map(columns, fn _ -> nil end) :)

rliebling commented 2 years ago

Sounds good - thanks! will get to this in a couple days. I assume nobody else is urgently waiting on this so the delay is fine.

rliebling commented 2 years ago

Ok - thinks this should be good now.

rliebling commented 2 years ago

Thank you both for your patience! You set a terrific example for the community (and open source) - when it undoubtedly would have been easier for you to just fix my initial attempts and commit it than to give me the feedback.