Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make arguments to engine optional #2477

Merged
merged 25 commits into from
Jul 12, 2024
Merged

Make arguments to engine optional #2477

merged 25 commits into from
Jul 12, 2024

Conversation

AndreasArvidsson
Copy link
Member

Checklist

  • [/] I have added tests
  • [/] I have updated the docs and cheatsheet
  • [/] I have not broken the cheatsheet

@pokey
Copy link
Member

pokey commented Jul 11, 2024

update from meet-up re null tree-sitter:

  • if tree-sitter is null, create DisabledLanguageDefinitions that just doesn't support any languages
  • remove tree-sitter debug prints
  • allow null to pass to parse tree action for now and just have null check there that throws

goal for all this stuff is to allow undefined components to pass into engine, and contain all resulting null checks to createEngine, and create a DisabledFoo in response. Note that third checkbox above fails this criterion, but we don't need to be dogmatic

@AndreasArvidsson
Copy link
Member Author

ready!

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow this PR does a lot. I started in for a quick review but there's too much going on to easily see the picture. I would be tempted to split it into 4 PRs, but how about as a compromise let's just split it into 2:

  1. CommandServerApi / HatTokenMap
  2. LanguageDefinitions / TreeSitter

I think that is a nice split point

See also the commit I just pushed, and comments below

@@ -27,7 +27,7 @@ export class CommandHistory implements CommandRunnerDecorator {
constructor(
private ide: IDE,
private storage: CommandHistoryStorage,
private commandServerApi: CommandServerApi | null,
private commandServerApi: CommandServerApi | undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer ever undefined right?

Suggested change
private commandServerApi: CommandServerApi | undefined,
private commandServerApi: CommandServerApi,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. This is constructed in the extension file and does not get the disabled default implementation from the engine

packages/cursorless-engine/src/core/Debug.ts Outdated Show resolved Hide resolved
@AndreasArvidsson
Copy link
Member Author

Ok this pull request has now been stripped off the Tree sitter stuff

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good with one minor comment!

@pokey pokey added this pull request to the merge queue Jul 12, 2024
Merged via the queue into main with commit 4d68be9 Jul 12, 2024
15 checks passed
@pokey pokey deleted the engineInterface branch July 12, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants