Implement IJupyterVariables for the debugger - #11543
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11543 +/- ##
==========================================
+ Coverage 60.45% 60.51% +0.06%
==========================================
Files 618 617 -1
Lines 33774 33609 -165
Branches 4763 4727 -36
==========================================
- Hits 20419 20340 -79
+ Misses 12347 12287 -60
+ Partials 1008 982 -26
Continue to review full report at Codecov.
|
| // Licensed under the MIT License. | ||
| 'use strict'; | ||
| import type { nbformat } from '@jupyterlab/coreutils'; | ||
| import type { JSONObject } from '@phosphor/coreutils'; |
There was a problem hiding this comment.
This file is what used to be jupyterVariables. I didn't change it at all. It is used if the run by line experiment is turned off.
| // Run the get dataframe rows script | ||
| if (!this.debugService.activeDebugSession) { | ||
| // No active server just return the unchanged target variable | ||
| return {}; |
There was a problem hiding this comment.
wouldn't this be targetVariable? Based on the comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Actually no, the comment is just wrong. I'll fix the comment. This is the row data. If it can't be fetched, it should return an empty json object.
In reply to: 418778956 [](ancestors = 418778956,418774471)
| // If using the interactive debugger, update our variables. | ||
| if (message.type === 'response' && message.command === 'variables') { | ||
| // tslint:disable-next-line: no-suspicious-comment | ||
| // TODO: Figure out what resource to use |
There was a problem hiding this comment.
Just checking (since I use TODOs like this as well) this is something that is planned for a later checkin. Not something that you need now?
There was a problem hiding this comment.
It works right now, so not worrying about it. But at some point yes. I need to somehow figure out the active resource. It's messy though. The debug service doesn't have the concept of a file that it's debugging. Maybe the filtering for variables should be done at a different location.
In reply to: 418775579 [](ancestors = 418775579)
Don Jayamanne (DonJayamanne)
left a comment
There was a problem hiding this comment.
Lots to review, will re-visit on monday.
| @inject(IJupyterVariables) @named(Identifiers.DEBUGGER_VARIABLES) private debugVariables: DebugAdapterTracker | ||
| ) {} | ||
| public activate(): Promise<void> { | ||
| this.disposables.push(this.debugService.registerDebugAdapterTrackerFactory('python', this)); |
There was a problem hiding this comment.
We have a constant for python. I think its PYTHON_LANGUAGE or something. Please could we use that.
There was a problem hiding this comment.
| _notebook: INotebook, | ||
| start: number, | ||
| end: number | ||
| ): Promise<JSONObject> { |
There was a problem hiding this comment.
Why do we need to use JSONObject, why not just {} or some other type?
There was a problem hiding this comment.
Yeah I guess it doesn't need that. It was that way before. I'll change it as that's much simpler.
In reply to: 418783160 [](ancestors = 418783160)
|
And thanks for splitting the PR. |
| data: { | ||
| executionCount: arg.prevState.currentExecutionCount, | ||
| sortColumn: 'name', | ||
| sortAscending: true, |
There was a problem hiding this comment.
Is this clearing sort order on refresh? I would think that would stay from the previous state.
There was a problem hiding this comment.
There was a problem hiding this comment.
Forgot about that. Sounds good.
|
Kudos, SonarCloud Quality Gate passed!
|
| constructor( | ||
| @inject(IDebugService) private debugService: IDebugService, | ||
| @inject(IDisposableRegistry) private disposables: IDisposableRegistry, | ||
| @inject(IJupyterVariables) @named(Identifiers.DEBUGGER_VARIABLES) private debugVariables: DebugAdapterTracker |
There was a problem hiding this comment.
I love this... simple OOP, simple abstraction and now yay we have a new implementation.
For #11542
I need this work for the Run By Line mvp and part of implementing it will work for both interactive and the run by line, so I did this first.