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

[WIP] Partial merge of ModelDB proposal. #2039

Merged
merged 16 commits into from Apr 19, 2017
Merged

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 11, 2017

A selective merge of #2021 which includes just the model database proposal, and should be a bit easier to read through.

delete cell.metadata['trusted'];
trusted.changed.connect((value, args) => {
Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

As a general pattern we don't connect signals to anonymous functions, because they won't be cleared by Signal.clearData(this).

oldValue,
newValue
});
(this.modelDB.get('mimeType') as IObservableValue).set(newValue);
Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

What do you think about adding db.setValue('mimeType', foo) and db.getValue('foo') to avoid this verbose pattern?

Copy link
Member Author

@ian-r-rose ian-r-rose Apr 11, 2017

Choose a reason for hiding this comment

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

Sure, that works. It would also be possible to just keep a reference to the IObservableValue and call set on that.

*
* @param path: the path for the object.
*
* @returns the object that wsa created.
Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

was*

@@ -204,7 +209,7 @@ class CellList implements IObservableUndoableVector<ICellModel> {
*/
set(index: number, cell: ICellModel): void {
// Generate a new uuid for the cell.
let id = uuid();
let id = (cell as any).modelDB.basePath;
Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

Why not add the id as a readonly attribute of the cell model?

Copy link
Member Author

@ian-r-rose ian-r-rose Apr 11, 2017

Choose a reason for hiding this comment

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

Yes, this pattern was more of a temporary hack until we could have a discussion about it. Do you see any negative consequences adding a cell id to the model?

Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

None for the live model. Adding it to nbformat is a whole other can of worms.

Copy link
Member Author

@ian-r-rose ian-r-rose Apr 11, 2017

Choose a reason for hiding this comment

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

Sounds good.

@@ -322,6 +330,12 @@ namespace NotebookModel {
* The default is a shared factory instance.
*/
contentFactory?: IContentFactory;


Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

Extraneous space

@@ -398,6 +423,9 @@ namespace NotebookModel {
if (options.contentFactory) {
options.contentFactory = this.codeCellContentFactory;
}
if(this._modelDB) {
Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

missing space after if here and below

} else {
this._serialized.set(this.toJSON());
}
this._serialized.changed.connect((obs, args) => {
Copy link
Member

@blink1073 blink1073 Apr 11, 2017

Choose a reason for hiding this comment

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

more anonymous signal connections

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Apr 12, 2017

Okay, ready for another round of review.

@@ -10,6 +10,10 @@ import {
} from '@phosphor/signaling';

import {
utils
Copy link
Member

@blink1073 blink1073 Apr 13, 2017

Choose a reason for hiding this comment

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

These utils going away, uuid is available from coreutils directly.

@@ -165,38 +188,35 @@ class CellModel extends CodeEditor.Model implements ICellModel {
readonly stateChanged = new Signal<this, IChangedArgs<any>>(this);

/**
* The id for the cell.
*/
get id(): string {
Copy link
Member

@blink1073 blink1073 Apr 13, 2017

Choose a reason for hiding this comment

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

Can be a public readOnly

*/
cell?: nbformat.IBaseCell;

export interface IOptions extends CellModel.IOptions {
Copy link
Member

@blink1073 blink1073 Apr 13, 2017

Choose a reason for hiding this comment

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

Should be a new line after export

* The vector can only store objects that are simple
* JSON Objects and primitives.
*/
createUndoableVector<T extends JSONValue>(path: string): IObservableUndoableVector<T>;
Copy link
Member

@blink1073 blink1073 Apr 13, 2017

Choose a reason for hiding this comment

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

Should we just support undo/redo for all object types since they all use JSON values?

Copy link
Member Author

@ian-r-rose ian-r-rose Apr 13, 2017

Choose a reason for hiding this comment

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

Sure. As it is, createVector is not used anywhere in the codebase.

*
* @param value: the value to set at the path.
*/
set(path: string, value: IObservable): void {
Copy link
Member

@blink1073 blink1073 Apr 13, 2017

Choose a reason for hiding this comment

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

Should this be protected?

Copy link
Member Author

@ian-r-rose ian-r-rose Apr 13, 2017

Choose a reason for hiding this comment

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

In this implementation it is being called by other instances of ModelDB (but not derived from it). It is not in the public IModelDB interface, so should not be callable by things that consume that interface.

@@ -398,6 +423,12 @@ namespace NotebookModel {
if (options.contentFactory) {
options.contentFactory = this.codeCellContentFactory;
}
if (this._modelDB) {
if (!options.id) {
options.id = utils.uuid();
Copy link
Member

@blink1073 blink1073 Apr 13, 2017

Choose a reason for hiding this comment

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

Can also use uuid from coreutils here

@blink1073
Copy link
Member

@blink1073 blink1073 commented Apr 15, 2017

I'm happy with the current status, but I think this PR should include tests for the base model DB implementation. I'm wary of adding something so foundational without explicit tests.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Apr 15, 2017

Great! I'll add some tests.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Apr 18, 2017

I have been using slashes (/) as delimiters for path components, but have been thinking of switching to dots (.), which is closer to the path spec for the racerjs realtime backend that I am playing with. For the in-memory and Google-Drive implementations it should not make a difference. Any technical reasons to favor one over the other? Thoughts:

  • dots are closer to a object-ish representation of the DB.
  • slashes are closer to a filesystem-ish representation.
  • slashes are reserved characters in POSIX paths, dots are allowed.
  • in either case, I will need to document which characters are expected to be reserved.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Apr 18, 2017

Does the path include the file name (which would have dots)?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Apr 18, 2017

No, it does not. I am not aware of technical problems with either, just wanted to give it some thought before baking it in.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Apr 18, 2017

Okay, then 👍 to dots.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Apr 19, 2017

Ugh, it looks like our Appveyor build got bit by the tornado 4.5 release. Is this ready to go otherwise?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Apr 19, 2017

Yes, otherwise should be good.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Apr 19, 2017

Great, thanks!

@blink1073 blink1073 merged commit afc241f into jupyterlab:master Apr 19, 2017
1 of 2 checks passed
@blink1073 blink1073 mentioned this pull request Apr 21, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants