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

Export Notebook for Web: call JupyterLab#ContentManager directly to simplify JupyterKernelSession #13577

Closed
Tracked by #12832
DonJayamanne opened this issue May 28, 2023 · 0 comments · Fixed by #13736
Assignees
Labels
debt Code quality issues on-testplan
Milestone

Comments

@DonJayamanne
Copy link
Contributor

DonJayamanne commented May 28, 2023

export interface IJupyterKernelSession extends IBaseKernelSession<'remoteJupyter' | 'localJupyter'> {
    invokeWithFileSynced(contents: string, handler: (file: IBackupFile) => Promise<void>): Promise<void>;
    createTempfile(ext: string): Promise<string>;
    deleteTempfile(file: string): Promise<void>;
    getContents(file: string, format: Contents.FileFormat): Promise<Contents.IModel>;
}

All of the above methods in the interface are only used in one place and one context.
Hence we should try to avoid adding these methods to a very generic interface,

Better to just expose the ContentManager and move all of the code into the Export or other layer rathe than keeping creation/deletion/reading of files in the IJupyterKernelSession class/interface.

Suggestions:

  • Expose the ContentManager of IJupyterSessionManager, this way we do not need anything IJupyterKernelSession.

After all, all of the above 4 methods on the session has nothing to do with the kernel session, but its about content management.

Also see #13578

@DonJayamanne DonJayamanne added bug Issue identified by VS Code Team member as probable bug debt Code quality issues labels May 28, 2023
@DonJayamanne DonJayamanne self-assigned this May 28, 2023
@DonJayamanne DonJayamanne changed the title Refactor Export on Web to remove unnecessary methods on IJupyterKernelSession Export Notebook for Web: call JupyterLab#ContentManager directly to simplify JupyterKernelSession Jun 12, 2023
@DonJayamanne DonJayamanne added this to the June 2023 milestone Jun 12, 2023
@DonJayamanne DonJayamanne removed the bug Issue identified by VS Code Team member as probable bug label Jun 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues on-testplan
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant