Fix test fixture upgrade scripts #942

SimeonC · 2022-09-08T12:20:56Z

Fixed the v1 to v2 script to preserve order and not drop any values.

Use normalisation code for updating fixtures in

cursorless/src/test/suite/recorded.test.ts

Lines 130 to 138 in a4cee96

const outputFixture = {

...fixture,

finalState: undefined,

decorations: undefined,

returnValue: undefined,

thrownError: { name: error.name },

};

await fsp.writeFile(file, serialize(outputFixture));

(filed Make fixture update code more consistent #1168 to follow up on this one)

pokey

Looks great thank you! Left a few minor comments

pokey · 2022-09-08T12:50:58Z

.vscode/settings.json

@@ -22,5 +22,6 @@
 		"pojo",
 		"subword"
 	],
-	"files.eol": "\n"
+	"files.eol": "\n",
+	"typescript.tsdk": "node_modules/typescript/lib"


This is nice; let's upgrade Typescript in this PR as well though I think

I upgraded TS as well, but I left the website at 4.5 as the typedocs only supports up to 4.5 and I didn't want to risk breaking the website!

Hmm I think that will be a problem, because we run typedocs on all the code in cursorless to generate api docs. We should prob pin both to the same version

When we switch to nx we'll only have one set of versions for everything fwiw

I can try upgrade it but I'm not familiar with docasaurus and it's typedocs so I'd need someone to double check that. It may end up being a much bigger upgrade - if it does need more than a simple dependency upgrade do we want to just do that here or make a different PR for upgrading the website deps.

I upgraded TS in website, it seems working to me as far as I can tell. The types are getting output in the contributor docs but I'm not sure they're 100% correct.

We should probably also make VSCode actually ask whether we want to use it too, otherwise nobody will actually switch to it.

Suggested change

"typescript.tsdk": "node_modules/typescript/lib"

"typescript.enablePromptUseWorkspaceTsdk": true,

"typescript.tsdk": "node_modules/typescript/lib"

@auscompgeek that sounds reasonable to me. Any objections @SimeonC ?

Re types in website; I just kicked off a deploy preview. I'll take a look at docs once it's out to see if they look sane

Huh. Unfortunately this PR breaks the api docs. For example, in the docs for runCommand, see how on production there are a bunch of links in the description, but these seem to have disappeared for some reason in the deploy preview 😕

pokey · 2022-09-08T12:51:18Z

src/definitions/utils.d.ts

+type GetOptional<T> = {
+  [K in keyof T as Pick<T, K> extends Required<Pick<T, K>> ? never : K]: T[K];
+};
+
+type GetRequired<T> = {
+  [K in keyof T as Pick<T, K> extends Required<Pick<T, K>> ? K : never]: T[K];
+};
+
+type UnionUndefined<T> = {
+  [K in keyof T]: T[K] | undefined;
+};


Did we want to expose these globally? From what I can tell, everything in this file gets exposed globally, but these are just helper functions for EnforceUndefined

Personally, I've never really bothered thinking about that. If that's an issue we could just change it to a manual import, or I could use a namespace for the utils - whichever you prefer.

With the namespace way Utils would be globally available but it does clutter it less, we could put all the types in the Utils namespace so the only "globally" available type is the Utils variable.

declare namespace Utils { type GetOptional ... } type EnforceUndefined = Utils.GetOptional... // some other file EnforcedUndefined<... // This is OK GetOptional<... // would error Utils.GetOptional<... // This is also OK

@pokey What approach do you want me to take here? import { EnforceUndefined } from '...' or the Utils namespace or just leave it as is?

Tbh I'd be tempted to just require users to import EnforceUndefined. With auto-import, that's not particularly onerous, and it makes the code a bit better self-documenting, as you can see in the imports where it's coming from. I'm not a huge fan of globally available symbols

src/scripts/transformRecordedTests/transformations/reorderFields.ts

Didn’t upgrade the website as the typegen package requires ts 4.5

…ds.ts Update key order Co-authored-by: Pokey Rule <[email protected]>

auscompgeek

Looking pretty good to me too!

src/testUtil/cleanUpTestCaseCommand.ts

pokey · 2022-09-18T14:26:29Z

I think we should split this PR into two:

one to fix the fixture upgrade scripts, and
one to upgrade typescript and refactor type hints in makeGraph.ts

I believe these two pieces are completely orthogonal, and the former is more imperative, while the latter is causing the problems. Make sense?

SimeonC · 2022-09-19T10:57:18Z

Sure we could do that. I haven't had the time to come back to look at whether the fix for the docs is just some missing new config or it needs a refactor.
Sorry I'm taking so long to get round to finishing this - don't mind if someone else who has more time than me picks this up and splits it into 2 PRs.

Co-authored-by: David Vo <[email protected]>

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

pokey · 2022-12-09T18:21:12Z

ok @SimeonC I factored out the typescript stuff into #1166 and #1167, and made some tweaks. lmk if you have any objections

SimeonC · 2022-12-11T11:09:51Z

Thanks for updating and completing it, looks good to me!

- closes #945 - closes #1044 - depends on #1322 - depends on #1327 ## Checklist - [x] Split into smaller PRs for ease of review - [x] Try DX tests again #1289 - [x] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [x] File issue for speeding up cheatsheet / making it not optional locally #1307 - [x] Set package manager in package.json https://nodejs.org/api/packages.html#packagemanager - [x] Re-add lint rule to prevent api types from importing anything - [x] Try local install - [x] Make sure cheatsheet works - [x] Make sure local extension build depends on building cheatsheet, or allows no cheatsheet - [x] Figure out why we're getting so many extra `internal` modules - [x] Update #931 to indicate we're now patching instead of swizzling - [x] Make sure lint rules are actually running in CI - [x] Check that we've addressed everything in #945 - [x] Incorporate changes from #1166 - [x] ~~Make sure this PR doesn't break doc links; see #942 (comment) There's no way to avoid these links getting broken if we upgrade typedoc; filed #1304 to track - [x] Revert #1284 - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [x] I have not broken the cheatsheet - [x] Generate cursorless-vscode package.json automatically, and change the name we use to `@cursorless/cursorless-vscode` in the `package.json` checked into source control - [x] Be sure to update meta-updater to check for that instead of name `cursorless` (see fixme in its `index.ts`) ## Desiderata - From #1289 ### Correctly flagging errors - [x] `D1`: It is a compile error to import external packages that are not listed in `package.json` - [x] `D1.1`: `vscode` (for some reason this one behaves strangely) - [x] `D1.2`: Other packages - [x] `D2`: It is **not** a compile error to import from local packages that you depend on, using our preferred syntax (eg `@cursorless/foo`) - [x] `D3`: It is a compile error to import from local packages that you depend on, **not** using our preferred syntax (eg `../foo`) - [x] `D4`: It is a compile error to import from local packages that you do **not** depend on, either - [x] `D4.1`: using our non-preferred syntax (eg `../packages/foo`), or - [x] `D4.2`: using preferred syntax (eg `@cursorless/foo`) - [ ] `D5`: It is a compile error to import from anything other than `index.ts` in another module - [x] `D5.1`: **no** `@cursorless/foo/bar` - [x] `D5.2`: **no** `../foo/bar` - [x] `a`: Even if you depend on the module - [ ] `b`: Even if `bar` was re-exported in `foo/index.ts`. In that case it should be required to import `@cursorless/foo`. Untested, but probably works, and I ran out of steam 😅 - [x] `D6`: It is **not** a compile error to import external packages listed in `package.json` - [x] `D6.1`: `vscode` (for some reason this one behaves strangely) - [x] `D6.2`: Other packages ### Auto-import - [x] `D7`: Auto-import doesn't import from external packages not listed in `package.json` - [x] `D7.1`: `vscode` (for some reason this one behaves strangely) - [x] `D7.2`: Other packages - [x] `D8`: Auto-import imports from external packages listed in `package.json` - [x] `D8.1`: `vscode` (for some reason this one behaves strangely) - [x] `D8.2`: Other packages - [x] `D9`: Auto-import imports from local packages that you depend on, using our preferred syntax (eg `@cursorless/foo`) - [x] `D10`: Auto-import doesn't import from local packages that you don't depend on - [x] `D11`: Auto-import doesn't import from anything other than `index.ts` in another package (eg **no** `@cursorless/foo/bar`) - [x] `D11.1`: When you depend on the package - [x] `D11.2`: When you don't depend on the package ### Other DX - [x] `D12`: Find references across projects when no file from the referencing project is open - [x] `D13`: Rename across projects when no file from the referencing project is open - [x] `D14`: Jump to definition across projects - [ ] `D15`: It is easy to move a file from one package to another - [ ] `D16`: It is easy to create a new package - [ ] `D17`: Breakpoints work in Cursorless extension - [x] `D18`: Breakpoints work on Cursorless root website - [ ] `D19`: Breakpoints work on Cursorless docs website

Fix test fixture upgrade scripts

e4dfabf

SimeonC requested a review from pokey as a code owner September 8, 2022 12:20

Merge branch 'main' into fix-test-case-upgrader

b2240b9

pokey reviewed Sep 8, 2022

View reviewed changes

SimeonC and others added 2 commits September 9, 2022 10:18

Upgrade typescript

415fc9f

Didn’t upgrade the website as the typegen package requires ts 4.5

Update src/scripts/transformRecordedTests/transformations/reorderFiel…

a756baf

…ds.ts Update key order Co-authored-by: Pokey Rule <[email protected]>

auscompgeek reviewed Sep 9, 2022

View reviewed changes

src/testUtil/cleanUpTestCaseCommand.ts Outdated Show resolved Hide resolved

SimeonC and others added 3 commits September 12, 2022 12:00

Update website TS version and small style change

599368b

Merge branch 'main' into fix-test-case-upgrader

6fc95aa

Kick CI

a328448

pokey added the help wanted Extra attention is needed label Sep 20, 2022

pokey mentioned this pull request Oct 20, 2022

Upgrade typedoc #1044

Closed

pokey and others added 6 commits December 9, 2022 16:54

Update .vscode/settings.json

22677b8

Co-authored-by: David Vo <[email protected]>

Merge branch 'main' into pr/SimeonC/942

f169241

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

c0d73d0

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

Factor orthogonal work into separate PRs

653a0b8

Tweaks

5de5261

Fix test case transformer

7a85b60

pokey mentioned this pull request Dec 9, 2022

Make fixture update code more consistent #1168

Open

tweak readme

9c07b70

pokey requested a review from AndreasArvidsson December 9, 2022 18:21

AndreasArvidsson approved these changes Dec 11, 2022

View reviewed changes

This was referenced Dec 12, 2022

Upgrade typescript and use it for vscode TS server #1166

Closed

Clean up makeGraph.makeGetter #1167

Closed

Merge branch 'main' into fix-test-case-upgrader

1ae51ff

pokey approved these changes Dec 12, 2022

View reviewed changes

pokey merged commit c9e06d7 into cursorless-dev:main Dec 12, 2022

This was referenced Mar 9, 2023

Migrate to a pnpm monorepo #1281

Merged

Fix links stripped from docs during Typedoc link resolution #1304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test fixture upgrade scripts #942

Fix test fixture upgrade scripts #942

SimeonC commented Sep 8, 2022 •

edited by pokey

Loading

pokey left a comment

pokey Sep 8, 2022

SimeonC Sep 9, 2022

pokey Sep 9, 2022

SimeonC Sep 9, 2022

SimeonC Sep 12, 2022

auscompgeek Sep 12, 2022

pokey Sep 12, 2022 •

edited

Loading

pokey Sep 12, 2022

pokey Sep 8, 2022 •

edited

Loading

SimeonC Sep 9, 2022

SimeonC Sep 12, 2022

pokey Sep 12, 2022

auscompgeek left a comment

pokey commented Sep 18, 2022

SimeonC commented Sep 19, 2022

pokey commented Dec 9, 2022

SimeonC commented Dec 11, 2022

	const outputFixture = {
	...fixture,
	finalState: undefined,
	decorations: undefined,
	returnValue: undefined,
	thrownError: { name: error.name },
	};

	await fsp.writeFile(file, serialize(outputFixture));

	"typescript.tsdk": "node_modules/typescript/lib"
	"typescript.enablePromptUseWorkspaceTsdk": true,
	"typescript.tsdk": "node_modules/typescript/lib"

Fix test fixture upgrade scripts #942

Fix test fixture upgrade scripts #942

Conversation

SimeonC commented Sep 8, 2022 • edited by pokey Loading

pokey left a 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

pokey Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pokey Sep 8, 2022 • 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

auscompgeek left a comment

Choose a reason for hiding this comment

pokey commented Sep 18, 2022

SimeonC commented Sep 19, 2022

pokey commented Dec 9, 2022

SimeonC commented Dec 11, 2022

SimeonC commented Sep 8, 2022 •

edited by pokey

Loading

pokey Sep 12, 2022 •

edited

Loading

pokey Sep 8, 2022 •

edited

Loading