generalize code and markdown code lenses to be used with multiple languages #4876

amichuda · 2021-02-19T03:51:27Z

For #1238

This is still rough, but I wanted to make sure that things were being done correctly (pretty new at typescript)

Please let me know and I can make any changes

Thank you so much for your patience!

Features

Creates a codeLensExpression in the config to allow for both .py and .md files to be used in interactive window
Allows for file specific code and markdown Expressions, as well as defaultCellMarker
Can also be extended to more languages?

Problems

Units Tests fail for cellhashprovider.unit.test
Unit tests for datascience.unit.test fail for `dataScience.activate() because of this line:

const codeLensExpressions = this.configuration.getSettings(undefined).codeLensExpressions;
        codeLensExpressions.forEach(c => {
            this.extensionContext.subscriptions.push(
                 vscode.languages.registerCodeLensProvider({ language: c.language }, this.dataScienceCodeLensProvider)
             );
        });

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging. Not sure how to do this
Has telemetry for enhancements. Not sure how to do this
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).

…guages

rchiodo · 2021-02-19T17:09:33Z

src/client/datascience/cellMatcher.ts

+            language = 'python';
+        }
+
+        let codeLens = settings?.codeLensExpressions


Linter should have flagged this I believe. The 'let' should be a 'const'

rchiodo · 2021-02-19T17:10:51Z

src/client/datascience/cellMatcher.ts

-    constructor(settings?: IJupyterSettings) {
+    constructor(settings?: IJupyterSettings, language?: string) {
+
+        if (language) {


I think I'd rewrite this like so:

Suggested change

if (language) {

if (!language) {

language = python

}

And don't use the active text editor here. It should be used in the caller. Otherwise this code has an implicit dependency on the window namespace that isn't necessary.

How do you know that you want to use the active editor? There are cases where this is called on a non active editor

So that's why I'd pass in the language only. Let the caller decided what the language is.

Ah so you're saying, don't pass in a default value for language, just let language be an argument in the method and end it there?

Yeah I think so.

Make it the responsibility of the caller to determine it

How do you know that you want to use the active editor? There are cases where this is called on a non active editor

that wasn't done for any good reason, just don't know the best way to get the TextDocument object in each situation, so i defaulted to that.

No worries. The calling code should be able to make a better decision I think about whether or not to use the active editor to determine the language. You might have to trace back a few levels though (in the callers) to determine what they're actually doing. I think some of the code in cellFactory.ts has some intermediary functions that you might need to pass the language through as well.

So, sorry for the hand holding, but youre asking that I look through all instances of CellMatcher and see where a language argument is needed?

Well all of them should need one, but each caller will get it in different ways. So yes, you'd have to look for all callers of CellMatcher (you should be able to use the same go to references for the constructor to find them).

rchiodo · 2021-02-19T17:13:42Z

src/client/datascience/jupyter/jupyterImporter.ts

@@ -105,7 +105,9 @@ export class JupyterImporter implements INotebookImporter {
    };

    private get defaultCellMarker(): string {
-        return this.configuration.getSettings().defaultCellMarker || Identifiers.DefaultCodeCellMarker;
+        const language = window.activeTextEditor?.document.languageId


We actually have an abstraction around the VScode 'window' namepace called IDocumentManager. You'd inject an IDocumentManager into the constructor of the importer and use that here instead of the window namespace.

This abstraction allows us to have tests that can run without VS code being involved.

In this case, do I call the active text editor:

const language = this.DocumentManager.activeTextEditor?.document.languageId

where the constructor now has:

@inject(IDocumentManager) private readonly DocumentManager: IDocumentManager

Yes something like that. The importer may be called for things other than the active document though. It would be good to check where defaultCellMarker is used (under what circumstances). The caller of defaultCellMarker may have a file.

Where can I find the caller of defaultCellMarker?

If you have typescript reference tracking turned on, the IDE can tell you:

Or you can search for defaultCellMarker in that same file. It's a private member.

rchiodo · 2021-02-19T17:17:48Z

Looks good except for the cell matcher part with passing the language in.

Thanks for the PR.

rchiodo · 2021-02-19T17:19:25Z

Oh it seems you need to update .eslintrc.js. The linter step is giving this error:

Error: Files are being ignored that should be linted: src/test/datascience/cellMatcher.unit.test.ts
src/test/datascience/editor-integration/codewatcher.unit.test.ts
src/datascience-ui/react-common/settingsReactSide.ts
src/client/datascience/datascience.ts
src/client/datascience/cellMatcher.ts
src/client/datascience/editor-integration/codewatcher.ts
src/client/datascience/jupyter/jupyterImporter.ts

That means remove those files from the linter ignore list. You'll probably get linter errors then.

You can run lint on those files if you install the eslint extension (not sure if contributing.md mentions that or not. I'll go fix it if it doesn't)

rchiodo · 2021-02-19T17:22:04Z

The 'VSCode' tests are expected to fail until we can catch up to the new notebook APIs that VS code has been changing (we run our tests with insiders) (See this bug: #4796)

However the other tests should be passing pretty close to 100%. After you fix the build errors, they should go green.

amichuda · 2021-02-19T19:30:36Z

Getting a no-floating-promises error in datascience.ts

    private onSettingsChanged = () => {
        const settings = this.configuration.getSettings(undefined);
        const ownsSelection = settings.sendSelectionToInteractiveWindow;
        const editorContext = new ContextKey(EditorContexts.OwnsSelection, this.commandManager);
        **editorContext.set(ownsSelection).catch();**
    };

    private onChangedActiveTextEditor() {
        // Setup the editor context for the cells
        const editorContext = new ContextKey(EditorContexts.HasCodeCells, this.commandManager);
        const activeEditor = this.documentManager.activeTextEditor;

        if (activeEditor && activeEditor.document.languageId === PYTHON_LANGUAGE) {
            // Inform the editor context that we have cells, fire and forget is ok on the promise here
            // as we don't care to wait for this context to be set and we can't do anything if it fails
            **editorContext.set(hasCells(activeEditor.document, this.configuration.getSettings())).catch();**
        } else {
            **editorContext.set(false).catch();**
        }
    }

amichuda · 2021-02-19T19:33:51Z

have to fix a few more things as it turns out...

rchiodo · 2021-02-19T19:37:39Z

Getting a no-floating-promises error in datascience.ts

    private onSettingsChanged = () => {
        const settings = this.configuration.getSettings(undefined);
        const ownsSelection = settings.sendSelectionToInteractiveWindow;
        const editorContext = new ContextKey(EditorContexts.OwnsSelection, this.commandManager);
        **editorContext.set(ownsSelection).catch();**
    };

    private onChangedActiveTextEditor() {
        // Setup the editor context for the cells
        const editorContext = new ContextKey(EditorContexts.HasCodeCells, this.commandManager);
        const activeEditor = this.documentManager.activeTextEditor;

        if (activeEditor && activeEditor.document.languageId === PYTHON_LANGUAGE) {
            // Inform the editor context that we have cells, fire and forget is ok on the promise here
            // as we don't care to wait for this context to be set and we can't do anything if it fails
            **editorContext.set(hasCells(activeEditor.document, this.configuration.getSettings())).catch();**
        } else {
            **editorContext.set(false).catch();**
        }
    }

I think should be fixable using .ignoreErrors() instead of .catch()

…ode-jupyter into codelens-multiple-languages

amichuda · 2021-02-19T19:45:17Z

Getting a no-floating-promises error in datascience.ts

    private onSettingsChanged = () => {
        const settings = this.configuration.getSettings(undefined);
        const ownsSelection = settings.sendSelectionToInteractiveWindow;
        const editorContext = new ContextKey(EditorContexts.OwnsSelection, this.commandManager);
        **editorContext.set(ownsSelection).catch();**
    };

    private onChangedActiveTextEditor() {
        // Setup the editor context for the cells
        const editorContext = new ContextKey(EditorContexts.HasCodeCells, this.commandManager);
        const activeEditor = this.documentManager.activeTextEditor;

        if (activeEditor && activeEditor.document.languageId === PYTHON_LANGUAGE) {
            // Inform the editor context that we have cells, fire and forget is ok on the promise here
            // as we don't care to wait for this context to be set and we can't do anything if it fails
            **editorContext.set(hasCells(activeEditor.document, this.configuration.getSettings())).catch();**
        } else {
            **editorContext.set(false).catch();**
        }
    }

I think should be fixable using .ignoreErrors() instead of .catch()

Perfect.

Another issue, I just noticed is that the code cell expressions are sent to the interactive window as well, leading to python syntax error. I think it needs to be commented before being sent.

amichuda · 2021-02-19T19:50:16Z

Last merge fixed the issue with .catch().

Now there's an issue with encryptedStorage, "Property secrets does not exist on type 'IExtensionContext` :

public async store(service: string, key: string, value: string | undefined): Promise<void> {
        // On CI we don't need to use keytar for testing (else it hangs).
        if (IS_REMOTE_NATIVE_TEST && this.extensionContext.extensionMode !== ExtensionMode.Production) {
            this.testingState.set(`${service}#${key}`, value || '');
            return;
        }

        if (!value) {
            await this.extensionContext.secrets.delete(`${service}.${key}`);
        } else {
            await this.extensionContext.secrets.store(`${service}.${key}`, value);
        }
    }
    public async retrieve(service: string, key: string): Promise<string | undefined> {
        // On CI we don't need to use keytar for testing (else it hangs).
        if (IS_REMOTE_NATIVE_TEST && this.extensionContext.extensionMode !== ExtensionMode.Production) {
            return this.testingState.get(`${service}#${key}`);
        }
        // eslint-disable-next-line
        const val = await this.extensionContext.secrets.get(`${service}.${key}`);
        return val;
    }

rchiodo · 2021-02-22T17:14:23Z

Last merge fixed the issue with .catch().

Now there's an issue with encryptedStorage, "Property secrets does not exist on type 'IExtensionContext` :

public async store(service: string, key: string, value: string | undefined): Promise<void> {
        // On CI we don't need to use keytar for testing (else it hangs).
        if (IS_REMOTE_NATIVE_TEST && this.extensionContext.extensionMode !== ExtensionMode.Production) {
            this.testingState.set(`${service}#${key}`, value || '');
            return;
        }

        if (!value) {
            await this.extensionContext.secrets.delete(`${service}.${key}`);
        } else {
            await this.extensionContext.secrets.store(`${service}.${key}`, value);
        }
    }
    public async retrieve(service: string, key: string): Promise<string | undefined> {
        // On CI we don't need to use keytar for testing (else it hangs).
        if (IS_REMOTE_NATIVE_TEST && this.extensionContext.extensionMode !== ExtensionMode.Production) {
            return this.testingState.get(`${service}#${key}`);
        }
        // eslint-disable-next-line
        const val = await this.extensionContext.secrets.get(`${service}.${key}`);
        return val;
    }

This one is likely a merge required from main. We updated our version of VS code's API that we're using.

amichuda · 2021-06-11T03:23:04Z

Sorry for not getting to this for a long time. I got side tracked by grad school and couldn't get to it.

I'll try to get to this ASAP, but would also be completely open to having someone push this over the finish line!

…anguages

rchiodo · 2021-06-11T20:04:57Z

@amichuda I merged from main and it now builds again but I'm not sure the behavior is correct.

If I have markdown like so:

'```python'
print('foo')

It sends the code lens lines along with the python code. The interactive window shows this as an error:

I'll try debugging it later.

rchiodo · 2021-06-12T00:14:59Z

Had some time to debug. The code here was not stripping your delimiter so it was failing to be valid python code.

vscode-jupyter/src/client/datascience/jupyter/jupyterNotebook.ts

Line 901 in afddee1

code: cellMatcher.stripFirstMarker(code),

I made it so it could pick up the language id

codecov-commenter · 2021-06-12T01:17:45Z

Codecov Report

Merging #4876 (200e056) into main (88737d6) will increase coverage by 0%.
The diff coverage is 92%.

@@          Coverage Diff           @@
##            main   #4876    +/-   ##
======================================
  Coverage     71%     72%            
======================================
  Files        399     401     +2     
  Lines      26938   27085   +147     
  Branches    3964    3998    +34     
======================================
+ Hits       19374   19599   +225     
+ Misses      6188    5833   -355     
- Partials    1376    1653   +277

Impacted Files	Coverage Δ
src/client/common/types.ts	`100% <ø> (ø)`
...ient/datascience/editor-integration/codewatcher.ts	`68% <75%> (-1%)`	⬇️
.../datascience/interactive-common/interactiveBase.ts	`70% <75%> (-1%)`	⬇️
src/client/datascience/jupyter/jupyterImporter.ts	`65% <83%> (+<1%)`	⬆️
src/client/datascience/utils.ts	`68% <88%> (+4%)`	⬆️
src/client/datascience/cellMatcher.ts	`96% <92%> (-2%)`	⬇️
src/client/common/configSettings.ts	`89% <100%> (+<1%)`	⬆️
src/client/datascience/cellFactory.ts	`96% <100%> (+22%)`	⬆️
src/client/datascience/constants.ts	`99% <100%> (+<1%)`	⬆️
src/client/datascience/datascience.ts	`91% <100%> (+<1%)`	⬆️
... and 84 more

package.json

IanMatthewHuff · 2021-06-15T00:07:27Z

package.json

@@ -44,6 +44,7 @@
    ],
    "activationEvents": [
        "onLanguage:python",
+        "onLanguage:markdown",


Code looks pretty reasonable. But one more thing popping out here. The code expressions are added on activation via the lens manager. But it's only activating on python and markdown files loading. The languages that we add to codeExpressions would need to also register here as activation events. But that feels like it would have to be a dynamic list here. Not sure if that is possible.

Something to ask VS code then. Can a user add activation events to their settings.json for an extension.

rchiodo · 2021-07-26T16:33:02Z

There's still work to do here. We need some sort of 'end' marker for markdown cells or it won't get the correct cell ranges (nor have executable code)

amichuda · 2022-01-05T23:35:06Z

Sorry about getting back to this SUPER LATE. What's the best way to continue on this, rebase to master and finish the pull request?

rchiodo · 2022-01-08T01:16:00Z

Yeah a rebase would be the starting point. I don't think it works correctly yet though. At least if my memory is correct, markdown code isn't working because it doesn't have a way to match the 'end' for the markdown code. Normal code cells work because the end is the next cell divider. Markdown code is kinda in the middle of other stuff, so it needs a begin/end pair.

I don't think it should be too hard to match on ```python for codeStart and ``` for codeEnd in markdown files. But you'd have to add another setting in the settings.json to specify the 'end' matcher.

DonJayamanne · 2022-08-31T19:52:18Z

I'm going to close this PR as its been around for a while.
Please feel free to create a new PR once the issues have been addressed and/or the merge conflicts resolved.

generalize code and markdown code lenses to be used with multiple lan…

f82014a

…guages

amichuda requested a review from a team as a code owner February 19, 2021 03:51

rchiodo reviewed Feb 19, 2021

View reviewed changes

amichuda added 4 commits February 19, 2021 13:14

added language argument to cellmatcher and modified cellfactory

0c6f86a

updated linter ignore

71583ea

modified generateCell function to allow for language

e3700be

Merge branch 'main' into codelens-multiple-languages

9b18659

amichuda added 2 commits February 19, 2021 13:41

fixed bug where language isn't sent to cellMatcher

1f2bf76

Merge branch 'codelens-multiple-languages' of github.com:amichuda/vsc…

eff4e08

…ode-jupyter into codelens-multiple-languages

Merge remote-tracking branch 'upstream/main' into codelens-multiple-l…

351672f

…anguages

Fix the problem with not removing the first line for a markdown cell

07809ff

Update linting list

e06e827

rchiodo added 3 commits June 14, 2021 09:21

More linting

eb5dac7

More linting

aa14b94

Fix linter

b00f0c9

rchiodo added 3 commits June 14, 2021 12:00

Fix prettier issues

47afd4d

Fix cell matching and unit tests

b803a23

Fix regexes to skip markdown for markdown files

c44268e

IanMatthewHuff reviewed Jun 14, 2021

View reviewed changes

package.json Show resolved Hide resolved

Put back the old settings but deprecate them

200e056

IanMatthewHuff requested changes Jun 15, 2021

View reviewed changes

amichuda mentioned this pull request Jul 13, 2021

Running Stata code from VSCode kylebarron/stata_kernel#397

Open

rchiodo mentioned this pull request Oct 4, 2021

Better support for running code from other kernels/languages in interactive window? #7784

Closed

amichuda mentioned this pull request Jan 5, 2022

[Feature request] VSCode extension for Quarto quarto-dev/quarto-cli#149

Closed

teonbrooks mentioned this pull request Jul 28, 2022

Support for editing Plain text files (like Python, MyST and R Markdown-based) as notebooks #1237

Closed

DonJayamanne closed this Aug 31, 2022

-        if (language) {
+if (!language) {
+   language = python
+}

generalize code and markdown code lenses to be used with multiple languages #4876

generalize code and markdown code lenses to be used with multiple languages #4876

Conversation

amichuda commented Feb 19, 2021 • edited Loading

Features

Problems

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amichuda Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amichuda Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo commented Feb 19, 2021

rchiodo commented Feb 19, 2021

rchiodo commented Feb 19, 2021

amichuda commented Feb 19, 2021 • edited Loading

amichuda commented Feb 19, 2021

rchiodo commented Feb 19, 2021

amichuda commented Feb 19, 2021

amichuda commented Feb 19, 2021 • edited Loading

rchiodo commented Feb 22, 2021

amichuda commented Jun 11, 2021 • edited Loading

rchiodo commented Jun 11, 2021 • edited Loading

rchiodo commented Jun 12, 2021

codecov-commenter commented Jun 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo commented Jul 26, 2021

amichuda commented Jan 5, 2022

rchiodo commented Jan 8, 2022 • edited Loading

DonJayamanne commented Aug 31, 2022

amichuda commented Feb 19, 2021 •

edited

Loading

amichuda Feb 19, 2021 •

edited

Loading

amichuda Feb 19, 2021 •

edited

Loading

amichuda commented Feb 19, 2021 •

edited

Loading

amichuda commented Feb 19, 2021 •

edited

Loading

amichuda commented Jun 11, 2021 •

edited

Loading

rchiodo commented Jun 11, 2021 •

edited

Loading

codecov-commenter commented Jun 12, 2021 •

edited

Loading

rchiodo commented Jan 8, 2022 •

edited

Loading