Fix Notebook CI failures; pin vscode version #768

pokey · 2022-06-13T13:26:01Z

Fixes Fix CI errors related to notebook cells #767
Fixes Fix notebook cell selection #487
Adds backwards compatibility checks for old VSCode versions
Adds support for selecting a notebook cell in opposite split (will be tested in follow-up PR for Add tests for cross-split commands #721)

Related to #497

Checklist

I have added tests

for more information, see https://pre-commit.ci

auscompgeek · 2022-06-13T14:44:57Z

third-party-licenses.csv

 "immutability-helper@3.1.1","MIT","https://github.com/kolodny/immutability-helper","https://github.com/kolodny/immutability-helper/raw/master/LICENSE","cursorless"
 "jsonc-parser@2.3.1","MIT","https://github.com/microsoft/node-jsonc-parser","https://github.com/microsoft/node-jsonc-parser/raw/master/LICENSE.md","parse-tree"
-"mdast-util-find-and-replace@2.1.0","MIT","https://github.com/syntax-tree/mdast-util-find-and-replace","https://github.com/syntax-tree/mdast-util-find-and-replace/raw/master/license","website"
+"lodash@4.17.21","MIT","https://github.com/lodash/lodash","https://github.com/lodash/lodash/raw/master/LICENSE","cursorless"


Interesting that this list has changed, despite the affected packages not changing in package.json or yarn.lock.

.github/workflows/test.yml

pokey · 2022-06-13T15:03:58Z

package.json

 	"extensionKind": [
-		"workspace"
+		"workspace",
+		"ui"


Added this based on user requests. Should help with running in various VSCode contexts, even if command-server might not work there

pokey · 2022-06-13T15:04:11Z

package.json

 		"@types/semver": "^7.3.9",
 		"@types/sinon": "^10.0.2",
-		"@types/vscode": "^1.61.0",
+		"@types/vscode": "~1.61.0",


Pinned for now so that we don't break backwards compatibility

.github/workflows/test.yml

src/test/runTest.ts

pokey · 2022-06-13T17:50:49Z

src/util/notebook.ts

+  // FIXME: All these type casts are necessary because we've pinned VSCode
+  // version type defs.  Can remove them once we are using more recent type defs


I'm not in love with this stuff, but not sure how to avoid it if we want to maintain backwards compatibility with older VSCode versions

I'm not fully understanding why we can't bump the VSCode type defs, though. Can you walk through it?

Is this a "looking forward" thing, ie. these APIs will eventually be deprecated?

There is a step in the vsce package command that checks whether the minimum VSCode engine version is less than the VSCode type defs version. So if we use the most recent type defs, we will have to bump our VSCode minimum engine, which would force all users to upgrade to the very latest VSCode, which seems a bit aggressive

Maybe there's a way to disable that check?

auscompgeek · 2022-06-13T23:03:00Z

src/test/runTest.ts

-    const vscodeExecutablePath = await downloadAndUnzipVSCode();
+    // NB: We include the exact version here instead of in `test.yml` so that
+    // we don't have to update the branch protection rules every time we bump
+    // the legacy VSCode version.


Hmm, you could set the job name using the matrix, but I guess this works as well.

Alternatively we could have a job that requires test, but seems excessive for what we need.

How would I go about setting the job name using the matrix? I don't see that in the docs 🤔. Do I just refer to {{ matrix.vscode_version }} in the name field of the job? And then I guess I'd use some kind of conditional expression to check whether it's equal to "stable"?

I guess alternately I could keep the version in the matrix as legacy and then put some kind of ternary in an expression when we define the env variable VSCODE_VERSION in test.yml? I didn't see anything bout a ternary either tho.

I'm going to ship this one as is to get CI working again, but feel free to add comments here if you have a better solution

Will-Sommers · 2022-06-14T03:15:59Z

src/util/setSelectionsAndFocusEditor.ts


-    if (activeTextEditor == null) {
-      return;
+    if (lt(version, "1.68.0")) {


Might be overkill, but it might make sense to store this in a constants file. We should likely check lt and also ensure gt from some baseline.

constants makes sense. Not sure bout gt; we get that for free from the VSCode min engine thing above

Will-Sommers · 2022-06-14T03:18:39Z

src/util/setSelectionsAndFocusEditor.ts

+  );

-    const cellOffset = editorIndex - activeEditorIndex;
+  const activeTextEditor = window.activeTextEditor;


It seems like we do this in many cases, might be interesting to think if there are ways we can add an AOP type filter for various actions.

Sorry not sure I'm following. Fwiw looks like I don't use the active text editor below, so I can remove this null check. It was a holdover from a previous approach. But maybe that's not what you're referring to?

pokey added 2 commits June 13, 2022 14:24

Pin vscode version

c885bb7

Bump pin

8e1df44

pokey force-pushed the pokey/fix-notebook-focusing-with-new-vscode-version branch from 7ffc1c3 to 8e1df44 Compare June 13, 2022 13:35

pokey and others added 2 commits June 13, 2022 14:56

Wrap old focus notebook code in legacy function

e52cdeb

[pre-commit.ci] auto fixes from pre-commit.com hooks

0425984

for more information, see https://pre-commit.ci

auscompgeek reviewed Jun 13, 2022

View reviewed changes

pokey added 3 commits June 13, 2022 15:54

Fix notebook focusing

c1bf7fb

Test on later version of VSCode

a9ba904

revert licences csv

5ef1934

pokey marked this pull request as ready for review June 13, 2022 15:05

pokey commented Jun 13, 2022

View reviewed changes

pokey requested review from AndreasArvidsson and Will-Sommers June 13, 2022 15:06

Change Github action setup

815379c

pokey commented Jun 13, 2022

View reviewed changes

pokey added 2 commits June 13, 2022 18:59

Add NB

86aa69f

Fix import

5f75975

auscompgeek reviewed Jun 13, 2022

View reviewed changes

Will-Sommers reviewed Jun 14, 2022

View reviewed changes

Will-Sommers previously approved these changes Jun 14, 2022

View reviewed changes

Remove unused code

eb50740

pokey dismissed Will-Sommers’s stale review via eb50740 June 14, 2022 15:44

Add function to check legacy notebook version

26fc53b

pokey merged commit c55c281 into main Jun 14, 2022

pokey deleted the pokey/fix-notebook-focusing-with-new-vscode-version branch June 14, 2022 16:18

pokey mentioned this pull request Jun 22, 2022

Downgrade vscode engine #497

Closed

		// FIXME: All these type casts are necessary because we've pinned VSCode
		// version type defs. Can remove them once we are using more recent type defs

Uh oh!

Fix Notebook CI failures; pin vscode version #768

Fix Notebook CI failures; pin vscode version #768

Uh oh!

Conversation

pokey commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Will-Sommers Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pokey Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pokey commented Jun 13, 2022 •

edited

Loading

Will-Sommers Jun 14, 2022 •

edited

Loading

pokey Jun 14, 2022 •

edited

Loading