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

Add initial notebook data structure #4

Merged
merged 2 commits into from
Jan 8, 2021
Merged

Add initial notebook data structure #4

merged 2 commits into from
Jan 8, 2021

Conversation

jonatanklosko
Copy link
Member

@jonatanklosko jonatanklosko commented Jan 8, 2021

Notes:

  1. When prototyping I stored session_data on cells (for keeping OT changes; whether the cell has been executed; etc). Conceptually this is ephemeral session data, so thinking about it, it would be cleaner to store such metadata on session-level (e.g. a map from cell id to session data).
  2. We can distinguish cell types either by using a :type attribute or have a separate structs like Cell.Elixir, Cell.Markdown. One one hand markdown cells are simple and we will probably store them as annotations as per @jonklein's draft, on the other the cells have many similarities. For now I went with :type, but happy to hears opinions on that.
  3. The structs have random ids, so we can easily drop them when storing the file and generate new ids upon import.

@@ -0,0 +1,41 @@
defmodule LiveBook.Notebook.Cell do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if calling ElixirCell and MarkdownCell will be cleaner? Especially because they are often very distinct in terms of rendering and behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, I read the issues description right now. :) I am fine with making this decision later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I've been wondering about. Here's something I tried. If you think it's better, I'm happy to go with that.

Also, Cell.Elixir vs ElixirCell?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are fine! It is easier to decide later, so let's decide later. :)

%__MODULE__{
metadata: %{
name: "Untitled notebook",
version: @version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think version and name should be part of the structure. Generally speaking, metadata should be a map from (atom => term), we shouldn't really know what keys are actually in the metadata, as it can be anything. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Do we want to have the metadata already or maybe it falls into YAGNI?

Also version is more of the file format attribute, the structures version is basically LiveBook version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever you prefer. We will definitely need metadata at some point but anything that you will use all the time to show the document on the page, such as a name, then it should be part of the struct IMO.

@jonatanklosko
Copy link
Member Author

Thanks, I updated the PR. About storing session_data, I'm not entirely sure if keeping it separately is gonna be optimal (e.g. remembering to always add/remove entries whenever the related structs are added/removed), will see =)

@jonatanklosko jonatanklosko merged commit 464e30f into main Jan 8, 2021
@josevalim josevalim deleted the notebook branch January 8, 2021 16:25
@josevalim josevalim mentioned this pull request Nov 10, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants