-
Notifications
You must be signed in to change notification settings - Fork 3
Add an initial cell model #2
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,166 @@ | ||
// Copyright (c) Jupyter Development Team. | ||
// Distributed under the terms of the Modified BSD License. | ||
'use-strict'; | ||
|
||
import { | ||
IInputAreaViewModel | ||
} from 'jupyter-js-input-area'; | ||
|
||
import { | ||
IOutputAreaViewModel | ||
} from 'jupyter-js-output-area'; | ||
|
||
import { | ||
IObservableList | ||
} from 'phosphor-observablelist'; | ||
|
||
import { | ||
Widget | ||
} from 'phosphor-widget'; | ||
|
||
import './index.css'; | ||
|
||
|
||
/** | ||
* An enum which describes the type of cell. | ||
*/ | ||
enum CellType { | ||
/** | ||
* The cell contains code input. | ||
*/ | ||
Code, | ||
|
||
/** | ||
* The cell contains markdown. | ||
*/ | ||
Markdown, | ||
|
||
/** | ||
* The cell contains raw text. | ||
*/ | ||
Raw | ||
} | ||
|
||
|
||
/** | ||
* The arguments object emitted with the `stateChanged` signal. | ||
*/ | ||
export | ||
interface ICellChangedArgs<T> { | ||
name: string, | ||
oldValue: T; | ||
newValue: T; | ||
} | ||
|
||
|
||
/** | ||
* The definition of a model object for a base cell. | ||
*/ | ||
interface IBaseCellViewModel { | ||
|
||
/** | ||
* The type of cell. | ||
*/ | ||
cellType: CellType; | ||
|
||
/** | ||
* Tags applied to the cell. | ||
*/ | ||
tags?: IObservableList<string>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we restrict string to a subset with reg-exes in typescript ? I think if so we should restrict to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would need to be asserted dynamically at runtime. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i.e. not possible with the type system There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course for checking values at runtime. But I was more wondering if there was a syntax in the interface just for documentation purpose. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind, I confusing with JSON schema paternProperties. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine with For the metadat stuff, what about making the values be something like this:
That at least encodes the fact that we expect the data to be serializable On Tue, Nov 3, 2015 at 10:20 PM, Matthias Bussonnier <
Brian E. Granger There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, the javascript JSON.stringify function looks for a toJSON() function: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#toJSON()_behavior. |
||
|
||
/** | ||
* Get namespaced metadata about the cell. | ||
*/ | ||
getMetadata(namespace: string) : IObservableMap<string, JSONData>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aren't we getting rid of metadata on the view models anyway? (we got rid of it on the notebook view model). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, no I think it should be there still on both the cells and notebook. On Tue, Nov 3, 2015 at 7:56 PM, S. Chris Colbert [email protected]
Brian E. Granger There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What will the views use it for, that shouldn't otherwise be an actual property on the view model? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main place where cell metadata is edited is by the cell toolbars, which is entirely a View-level aspect of the cell. I think that all of our existing cell toolbars serve to simply edit that cell metadata. Because of this the cell toobars (which will probably be provided using extensions/extension points) should probably be passed the ViewModel of the cell. The metadata shouldn't be a single property on the ViewModel as we decided that we want to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not clear what will be edited by these toolbars, and why that data cant be formally typed, or at least exposed at a level below these view models. I think a concrete use case would help me understand here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In principle each library that starts to use a particular namespace in the
http://nbgrader.readthedocs.org/en/stable/user_guide/02_developing_assignments.html
All of this "data" is truly "metadata" in that it looses its meaning unless There are many other libraries that are using notebook/cell metadata in The problem is that all of the cell toolbar UIs will be provided by However, it very well may be that the celltoolbar extension authors will On Tue, Nov 3, 2015 at 8:27 PM, S. Chris Colbert [email protected]
Brian E. Granger There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not the type safety i'm worried about here; it's whether the cell/notebook view model is the right place for it. Are we expecting all implementations of Otherwise, the metadata could be accessible at a lower layer of the model hierarchy, and we can expose a way to get to access those models which will be available to the cell toolbar. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Brian E. Granger There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, this point is not about type-safety for me; it's about ease of implementing the interface and proper separation of concerns. The thing(s) that will be shared across plugin boundaries should be models, not view models (typically), and I can easily see this metadata living there. Here's some discussion on modelling a JSON value type (if you were curious): |
||
|
||
/** | ||
* The input area of the cell. | ||
*/ | ||
input: IInputAreaViewModel; | ||
|
||
/** | ||
* Whether a cell is deletable. | ||
*/ | ||
deleteable: boolean; | ||
|
||
/** | ||
* Whether a cell is mergable. | ||
*/ | ||
mergeable: boolean; | ||
|
||
/** | ||
* Whether a cell is splittable. | ||
*/ | ||
splittable: boolean; | ||
|
||
/** | ||
* Whether a cell is rendered. | ||
*/ | ||
rendered: boolean; | ||
|
||
/** | ||
* Whether the cell is marked for applying commands | ||
*/ | ||
marked: boolean; | ||
|
||
/** | ||
* Run the cell. | ||
*/ | ||
run(): void; | ||
} | ||
|
||
|
||
/** | ||
* The definition of a code cell. | ||
*/ | ||
export | ||
interface ICodeCellViewModel extends IBaseCellViewModel { | ||
|
||
/** | ||
* A signal emitted when state of the cell changes. | ||
*/ | ||
stateChanged: ISignal<ICodeCellViewModel, ICellChangedArgs<any>>; | ||
|
||
output: IOutputAreaViewModel; | ||
} | ||
|
||
|
||
/** | ||
* The definition of a raw cell. | ||
*/ | ||
export | ||
interface IRawCellViewModel extends IBaseCellViewModel { | ||
|
||
/** | ||
* A signal emitted when state of the cell changes. | ||
*/ | ||
stateChanged: ISignal<IRawCellViewModel, ICellChangedArgs<any>>; | ||
|
||
/** | ||
* The raw cell format. | ||
*/ | ||
format?: string; | ||
} | ||
|
||
|
||
/** | ||
* The definition of a markdown cell. | ||
*/ | ||
export | ||
interface IMarkdownCellViewModel extends IBaseCellViewModel { | ||
|
||
/** | ||
* A signal emitted when state of the cell changes. | ||
*/ | ||
stateChanged: ISignal<IMarkdownCellViewModel, ICellChangedArgs<any>>; | ||
} | ||
|
||
|
||
|
||
/** | ||
* A model consisting of any valid cell type. | ||
*/ | ||
export | ||
type ICellViewModel = ( | ||
IRawCellViewModel | IMarkdownCellViewModel | ICodeCellViewModel | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cellType
or justtype
? otherwise it will readcell.cellType
which seem a bit redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just grabbed it from the existing code where is it
cell_type
...I thinkthat
type
is a bit vague...On Tue, Nov 3, 2015 at 9:12 PM, Matthias Bussonnier <
[email protected]> wrote:
Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but
cell_type
is an historical artefact, likecode_cell
s havingsource
property, andmarkdown_cell
s havingtext
property. I think the global consistency of naming across the project is important. For exampleSteams
havename
s : if I had only to consider the JS API (not the msg spec), I would usetype
to be consistent.