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

Pass metadata from notebook to console #6063

Merged
merged 9 commits into from Apr 22, 2019
Merged

Conversation

@BoPeng
Copy link
Collaborator

@BoPeng BoPeng commented Mar 4, 2019

What it does: Allow runInConsole to pass cell metadata to the promptCell of console panel.

Why: In SoS notebook, each cell in the notebook can have its own kernel, saved in metadata['kernel']. When runInConsole is called to execute the cell in the console panel, the console panel needs to know the kernel information in addition to the code to execute.

What has been done: This simple patch sends metadata as an optional parameter to CodeConsole.inject.

NOTE: Tests pass locally but not on Azure etc. Note sure if they are related.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Mar 4, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@afshin afshin requested a review from jasongrout Mar 27, 2019
@afshin afshin added this to the 1.0 milestone Mar 27, 2019
Copy link
Contributor

@jasongrout jasongrout left a comment

Thanks, I think this looks good except for an unnecessary if check to remove, and possibly an iteration change.

let cell = this.createCodeCell();
cell.model.value.text = code;
if (metadata) {
Copy link
Contributor

@jasongrout jasongrout Apr 19, 2019

This is always true if the default is {}. How about we just delete the if check and let the for loop just iterate over an empty object if the default is passed in?

let cell = this.createCodeCell();
cell.model.value.text = code;
if (metadata) {
for (let key in metadata) {
Copy link
Contributor

@jasongrout jasongrout Apr 19, 2019

FYI, in general iterating over an object using in can iterate over inherited properties you typically don't want to iterate over. More common patterns these days are to use Object.keys(metadata).forEach(() => {cell.model.metadata.set(key, metadata[key]);});, or even:

for (let key of Object.keys(metadata)) {
         cell.model.metadata.set(key, metadata[key]); 
}

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 19, 2019

Great, thanks!

It looks like that last change introduced some changes from the previous version of prettier we were using. You merged master into this branch. You just need to update prettier by removing node_modules and doing another jlpm install, then those formatting reversions should go away.

@BoPeng
Copy link
Collaborator Author

@BoPeng BoPeng commented Apr 19, 2019

Done. I had to include the linter suggested changes to push, did not realize the change of linters.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 19, 2019

Thanks. The code looks good to me. I'm waiting for tests to pass.

@jasongrout jasongrout merged commit 7635934 into jupyterlab:master Apr 22, 2019
9 checks passed
@BoPeng BoPeng deleted the inject_metadata branch Apr 22, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 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