-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Feature/nextEdit #5921
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
Feature/nextEdit #5921
Conversation
…e-autocomplete Jacob/feature/call-alongside-autocomplete
…e-autocomplete Jacob/feature/call alongside autocomplete
Your cubic subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use cubic. |
✅ Deploy Preview for continuedev canceled.
|
core/nextEdit/IgnoreNextEdit.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like constants.ts
for consistency with the rest of the codebase?
import { shouldCompleteMultiline } from "../autocomplete/classification/shouldCompleteMultiline.js"; | ||
import { ContextRetrievalService } from "../autocomplete/context/ContextRetrievalService.js"; | ||
|
||
import { BracketMatchingService } from "../autocomplete/filtering/BracketMatchingService.js"; | ||
import { CompletionStreamer } from "../autocomplete/generation/CompletionStreamer.js"; | ||
import { postprocessCompletion } from "../autocomplete/postprocessing/index.js"; | ||
import { shouldPrefilter } from "../autocomplete/prefiltering/index.js"; | ||
import { getAllSnippets } from "../autocomplete/snippets/index.js"; | ||
import { renderPrompt } from "../autocomplete/templating/index.js"; | ||
import { GetLspDefinitionsFunction } from "../autocomplete/types.js"; | ||
import { AutocompleteDebouncer } from "../autocomplete/util/AutocompleteDebouncer.js"; | ||
import { AutocompleteLoggingService } from "../autocomplete/util/AutocompleteLoggingService.js"; | ||
import AutocompleteLruCache from "../autocomplete/util/AutocompleteLruCache.js"; | ||
import { HelperVars } from "../autocomplete/util/HelperVars.js"; | ||
import { AutocompleteInput, AutocompleteOutcome } from "../autocomplete/util/types.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok for now but long-term it's better to avoid importing implementation details from sibling services. This makes it easy to create circular dependencies, which will eventually lead to bugs and are often difficult to resolve.
For example, someone else could, in the future, import implementation details from nextEdit
into autocomplete and it generate a circular dependency that way.
It's usually better to move the reusable elements to a third directory that both siblings import from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't like to make deps this way either :( I didn't feel like making exact copies of these files inside the core/nextEdit
directory, although that would've been "safer" because the dependencies are contained within core/nextEdit
. My plan here is to phase these imports out gradually as we add more features to next edit.
@@ -83,12 +86,21 @@ export class ContinueCompletionProvider | |||
this.onError.bind(this), | |||
getDefinitionsFromLsp, | |||
); | |||
// NOTE: Only turn it on locally when testing (for review purposes). | |||
if (!IGNORE_NEXT_EDIT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming-wise, it's better to not have "negative" boolean flags. For example, instead of IGNORE_NEXT_EDIT
you could use IS_NEXT_EDIT_ACTIVE
. This helps avoid confusion and makes control flows simpler (no negation needed).
if (!IGNORE_NEXT_EDIT) { | |
if (IS_NEXT_EDIT_ACTIVE) { |
…e-autocomplete Revision on #5921 per PR review
Description
Merge #5919 and #5920.
5919:
Initial commits. Next edit module is created and is called alongside autocomplete. This is extremely rudimentary with no custom logic - the code is shamelessly copied over from the core/autocomplete directory, and all imports are pointing back to that directory. In the future, when we start adding more custom algorithms and types, we will slowly cut ties to the core/autocomplete directory and create our own internal definitions.
For now, this works only inside vscode. Jetbrains should require a custom message to some endpoint like "nextEdit/predict".
5920:
Turn off next edit for other users until it is ready.
This only targets vscode -- we need another check for jetbrains. I don't see any message endpoints defined for next edit for the jetbrains extension, so we should be good for now.
Checklist
Screenshots
[ For visual changes, include screenshots. Screen recordings are particularly helpful, and appreciated! ]
Tests
[ What tests were added or updated to ensure the changes work as expected? ]