Update gather functionality to support 0.4.1 of python-program-analysis #8219
Conversation
greazer
requested review from
IanMatthewHuff,
DonJayamanne,
rchiodo and
DavidKutu
Codecov Report
@@ Coverage Diff @@
## master #8219 +/- ##
==========================================
+ Coverage 59.23% 59.35% +0.11%
==========================================
Files 508 509 +1
Lines 23236 23267 +31
Branches 3750 3758 +8
==========================================
+ Hits 13765 13809 +44
+ Misses 8577 8559 -18
- Partials 894 899 +5
Continue to review full report at Codecov.
|
src/client/datascience/interactive-ipynb/nativeEditorProvider.ts
Outdated
Show resolved
Hide resolved
src/client/datascience/interactive-ipynb/nativeEditorProvider.ts
Outdated
Show resolved
Hide resolved
src/client/datascience/interactive-ipynb/nativeEditorProvider.ts
Outdated
Show resolved
Hide resolved
This was changed so it should have failed? Refers to: src/test/datascience/gather/gather.unit.test.ts:147 in af781c1. [](commit_id = af781c1, deletion_comment = False) |
src/client/datascience/interactive-ipynb/nativeEditorProvider.ts
Outdated
Show resolved
Hide resolved
| "description": "Enable code gather for executed cells. For a gathered cell, that cell and only the code it depends on will be exported to a new notebook.", | ||
| "scope": "resource" | ||
| }, | ||
| "python.dataScience.gatherToScript": { |
There was a problem hiding this comment.
This seems weird. Why not just always open a notebook if coming from a notebook and always open a file if coming from a file? For interactive window users, they'd never find this command and would be put off by us opening a notebook
| @@ -1088,6 +1088,11 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp | |||
| this.notebook = await server.createNotebook(await this.getNotebookIdentity()); | |||
| } | |||
|
|
|||
| if (this.notebook) { | |||
| const uri: Uri = await this.getNotebookIdentity(); | |||
There was a problem hiding this comment.
await this.getNotebookIdentity [](start = 29, length = 30)
This second await is redundant. You could just cache the value from the first call.
src/client/datascience/jupyter/liveshare/guestJupyterNotebook.ts
Outdated
Show resolved
Hide resolved
| @@ -114,6 +119,14 @@ class MockJupyterNotebook implements INotebook { | |||
| public async dispose(): Promise<void> { | |||
| return Promise.resolve(); | |||
| } | |||
|
|
|||
| public gatherCode(_c: ICell): string | undefined { | |||
| @@ -44,52 +37,36 @@ export class GatherExecution implements IGatherExecution, INotebookExecutionLogg | |||
|
|
|||
| traceInfo('Gathering tools have been activated'); | |||
| } | |||
| public logExecution(vscCell: IVscCell): void { | |||
| const gatherCell = convertVscToGatherCell(vscCell); | |||
There was a problem hiding this comment.
So I'm a bit nervous that we lost our cloneDeep here. Technically convert should not change the vscCell, but I'm worried someone else might tweak it. Was it a perf issue? #ByDesign
There was a problem hiding this comment.
Talked with Rich, we'll take care of this before calling postExecute, so disregard this.
In reply to: 342290179 [](ancestors = 342290179)
|
Thanks for the help Rich! |
Because that function is called A LOT. Every time we update code lenses. I wanted to preserver the O(n) of it if possible. And in case there were bugs, this will only affect gather then. |
For #8202
Has a news entry file (remember to thank yourself!)Has sufficient logging.Has telemetry for enhancements.Unit tests & system/integration tests are added/updatedTest plan is updated as appropriatepackage-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)