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

Implement saving signal #5096

Merged
merged 7 commits into from Aug 14, 2018
Merged

Implement saving signal #5096

merged 7 commits into from Aug 14, 2018

Conversation

@declanvk
Copy link
Contributor

@declanvk declanvk commented Aug 10, 2018

Added a signal to the DocumentRegistry.IContext that will trigger on the beginning of a save operation and again on completion or error.

Added tests for the start and completed signal values, but I was unable to come up with a test that would trigger a error while saving.

@ellisonbg

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 11, 2018

Thanks! We could create a read only file (see https://stackoverflow.com/a/28492823) here and then attempt to save to it with different content.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Aug 12, 2018

Thanks @declanvk I will help review this shortly...

@ellisonbg ellisonbg self-assigned this Aug 13, 2018
@vidartf
Copy link
Member

@vidartf vidartf commented Aug 13, 2018

What is the intended use for this signal? Would it make sense to also include the path/name of the file being saved in the signal?

Edit: Nevermind, I see that the path is available on the sender!

@@ -861,6 +866,8 @@ export namespace DocumentRegistry {
addSibling(widget: Widget, options?: IOpenOptions): IDisposable;
}

export type SaveState = 'starting' | 'completed' | 'failed';
Copy link
Member

@afshin afshin Aug 13, 2018

How do you feel about making the tenses match?

export type SaveState = 'started' | 'completed' | 'failed';

Copy link
Member

@afshin afshin Aug 13, 2018

I could imagine an argument being made for special-casing 'starting' ... but my gut feeling is to make them consistent.

Copy link
Contributor Author

@declanvk declanvk Aug 13, 2018

Sounds good, I'll make the change

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Aug 13, 2018

Also change tense of enum string to be consistent
@@ -39,6 +40,12 @@ def _create_notebook_dir():
os.mkdir(osp.join(root_dir, 'src'))
with open(osp.join(root_dir, 'src', 'temp.txt'), 'w') as fid:
fid.write('hello')

readonly_filpath = osp.join(root_dir, 'src', 'readonly-temp.txt')
Copy link
Member

@afshin afshin Aug 13, 2018

readonly_filpath => readonly_filepath (missing an e)

/**
* A signal emitted on the start and end of a saving operation
*/
get saving(): ISignal<this, DocumentRegistry.SaveState> {
Copy link
Member

@afshin afshin Aug 13, 2018

While we are on semantics, I think i would call this saveState and the private var _saveState.

})
.then(
value => {
// Capture all sucess paths and emit completion
Copy link
Member

@afshin afshin Aug 13, 2018

sucess => success ... also, please could you format comments in full sentences (including periods at the end)? Thank you!

@@ -744,6 +744,11 @@ export namespace DocumentRegistry {
*/
fileChanged: ISignal<this, Contents.IModel>;

/**
* A signal emitted on the start and end of a saving operation
Copy link
Member

@afshin afshin Aug 13, 2018

Same thing, with a period at the end.

return value;
},
err => {
// Capture all error paths and emit failure
Copy link
Member

@afshin afshin Aug 13, 2018

Same thing, with a period at the end.

Copy link
Contributor

@ellisonbg ellisonbg left a comment

Looks good to me - review addressed.

@blink1073 blink1073 merged commit 1a85c99 into jupyterlab:master Aug 14, 2018
2 checks passed
@declanvk declanvk deleted the impl-saving-signal branch Aug 14, 2018
@blink1073 blink1073 mentioned this pull request Aug 17, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 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

5 participants