runme icon indicating copy to clipboard operation
runme copied to clipboard

Set "good" default metadata

Open christian-bromann opened this issue 2 years ago • 6 comments

Currently a Runme cell has the following metadata interface:

  export interface Metadata {
    background?: string
    interactive?: string
    closeTerminalOnSuccess?: string
    mimeType?: string
    ['runme.dev/name']?: string
  }

When we deserialize the markdown currently the metadata object can be empty which requires us to transform in our extension. Given that these capabilities are implemented here I would suggest to have runme sanitize the values and ensure it is passed on with good default values so that the interface becomes:

  export interface Metadata {
    // @defaults false
    background: boolean
    // @defaults: true
    interactive: boolean
    // @defaults: true
    closeTerminalOnSuccess: boolean
    // @defaults: "text/plain"
    mimeType: string
    // @defaults: random string
    ['runme.dev/name']: string
  }

I am also wondering why the name (runme.dev/name) has the runme prefix? Seems unnecessary to me. Any reasoning behind?

Wdyt?

christian-bromann avatar Dec 17 '22 01:12 christian-bromann

I am also wondering why the name (runme.dev/name) has the runme prefix? Seems unnecessary to me. Any reasoning behind?

It is possible that Metadata contains name as well if it's defined in the snippet, i.e. sh { name=my-name }. If it's not, only ['runme.dev/name'] is defined indicating that name was not defined but was autogenerated and corresponds to what runme ls would return. I am not sure if it's used but @sourishkrout asked for it.

I agree with the rest. It would be nice to keep codify metadata here.

adambabik avatar Dec 20 '22 11:12 adambabik

It is possible that Metadata contains name as well if it's defined in the snippet, i.e. sh { name=my-name }. If it's not, only ['runme.dev/name'] is defined indicating that name was not defined but was autogenerated and corresponds to what runme ls would return. I am not sure if it's used but @sourishkrout asked for it.

name is only set if the name was explicitly specified in the underlying markdown as part of the code block whereas runme.dev/name being strictly ephemeral (not ever being serialized to a markdown file) by it's prefix will allow the extension to call code blocks via the CLI that aren't explicitly named.

In the not so distant future, Runme should assist with naming code blocks during notebook-based editing. However, Runme should never name code block explicitly on its own.

The defaults look good to me. One thing to keep in mind is that bool-ish values are currently encoded as strings.

sourishkrout avatar Dec 20 '22 14:12 sourishkrout

name is only set if the name was explicitly specified in the underlying markdown as part of the code block whereas runme.dev/name being strictly ephemeral (not ever being serialized to a markdown file) by it's prefix will allow the extension to call code blocks via the CLI that aren't explicitly named.

I think for now the extension only really cares about the unique reference to a code cell (its unique identifier). Having the ability to name a code cell to identify it with a specific command is a great feature I think. Therefor how about:

  • have an id property on the Cell level that represents the unique id to reference it, e.g.
    {
      id: string
      metadata: Metadata
      languageId: string
      value: string
      kind: NotebookCellKind.Code
    }
    
  • allow users to define a name as alias to the unique id
  • if an alias exists in a previous code block, it is being ignored

Wdyt?

christian-bromann avatar Dec 21 '22 08:12 christian-bromann

I do see where you coming from and don't disagree. Perhaps we should just use id in lieu of name.

I would, however, suggest to put this on the list of requirements for "exec engine". The metadata as it exists now (runme.dev/* etc) was introduce to maintain the CLI button functionality. To a degree it was required to use the same name identifier as a user would in a terminal session.

@adambabik?

sourishkrout avatar Dec 21 '22 14:12 sourishkrout

There are two issues:

  1. Preserving UUIDs
  2. Generating UUID for new cells

Regarding the first issue, I think it might be ok not to preserve UUIDs at all between notebook openings. Just generate them during the deserialize pass every time the notebook is opened. It might be not the best idea in the future if we want to somehow keep track of cell executions. Preserving them is not clear at all at this moment - we don't have any mechanism for that.

Re the second point, the new cells introduced into the document are created in VS Code so generating an UUID would be in both places. It's not a big deal, we just need to agree on an algorithm.

I don't see a point to worry about this now. I would delay this decision until we need it. Note that the current names are unique within a single document.

adambabik avatar Dec 21 '22 17:12 adambabik

I would delay this decision until we need it.

Makes sense to me.

sourishkrout avatar Dec 28 '22 23:12 sourishkrout