feat: new /doc command with smart selection #1116

abeatrix · 2023-09-19T19:10:24Z

feat: add smart selection and update doc prompt

This PR focuses on updating the /doc command, featuring snart selection.

Add getActiveTextEditorSmartSelection method to editor interface for supporting "smart selection" on files that have lang server running as we rely on that to extract the ranges for each function / methods within a class
Update prompt for the /doc command to specify docstring should not contain code but docstring only
Introduce CommandRunner class that allows different services to run different commands async
Add mode field to cody commands config
Add insertMode and autoApply fields to FixupTask that allows fixup to auto apply suggestions returned by Cody instead of displaying Code Lenses
/doc command now uses fixup insert mode

What is smart selection?

Smart selection removes the need to manually highlight code before running the /doc and /test commands. Instead, it will automatically detect and run the selected command for the function nearest to your cursor position. For /doc, the docstring will be added to the top of the function through fixup.

Why?

Based on user testing conducted by Megan, many first-time command users were unaware of the requirement to highlight code before running commands. We can create a smoother onboarding experience for new users with smart selection. Allowing our commands to work seamlessly without manual code selection will lower the learning curve and provide a more intuitive first-time user experience.

However, it's important to note that smart selection should be treated as a fallback method, as it produces a selection based on guessing user intent. Manual selection truly reflects the user's real intent and should be used when possible. Moreover, smart selection may not work well for all languages, and depends on if users have language servers running in their editor. In cases where smart selection is unreliable, users should fall back to using the manual selection method to ensure accuracy.

Follow-up

improve /doc prompt
apply fixup automatically
indentation issues caused by fixup

Test plan

Update 2 - latest

autoapply-doc.mov

Update 1

smartselection.mov

Start Cody from this branch
Run /doc command in a function without selecting any code (but put your cursor inside a function)
Run /doc command with code selected
Repeat step 2 and 3 with /test command
Run other commands, and use chat -- they should work as expected

toolmantim

This is so great @abeatrix! I love we're triggering a fixup, and just modifying the code!

Indention is often incorrect when documenting indented code… maybe something we can improve in the future with @valerybugakov's tree-sitter tech? One thing we could try in the future is auto-applying too. But one step at a time! :)

Is it possible to adjust the prompt so that Cody doesn't describe "parameters" in places where it doesn't make sense, because I'm getting this when documenting some selected code:

I gave it a go locally with this diff:

diff --git a/lib/shared/src/chat/prompts/default-prompts.json b/lib/shared/src/chat/prompts/default-prompts.json
index f1852861..def4714b 100644
--- a/lib/shared/src/chat/prompts/default-prompts.json
+++ b/lib/shared/src/chat/prompts/default-prompts.json
@@ -2,7 +2,7 @@
   "commands": {
     "doc": {
       "description": "Generate code documentation",
-      "prompt": "Generate a comment documenting the parameters and functionality for the selected code. Only generate the documentation for the selected code, do not generate the code. Use the same documentation style in the file of the selected code to generate the comments. Pay attention to the file path of the selected code to make sure the comments are generated for the correct language. For example, use the JavaDoc documentation style to generate comments for .java files, or Python docstring using Python multi-line string for .py files. Do not include any other code or comments besides the docstring.",
+      "prompt": "Generate a comment briefly documenting the purpose of the selected code. If it takes parameters, document those. Only generate the documentation for the selected code, do not generate the code. Use the same comment style in the file of the selected code, or the most common style for that language. If no existing documentation comments exist, pay attention to the file path of the selected code to make sure the comments are generated in the style of that language. Do not include any other code or comments besides the comment.",
       "context": {
         "currentDir": true,
         "selection": true

And ended up with better results for the selected lines:

That appears to still document functions correctly too:

I don't have a good test suite / cases to verify it across more examples, so will defer to your prompt magic skills!

toolmantim · 2023-09-20T07:27:09Z

lib/shared/src/chat/prompts/default-prompts.json

      "context": {
        "currentDir": true,
        "selection": true
-      }
+      },
+      "mode": "insert"


Nice! Does this mean custom commands can do this too?

YES! Also takes "inline" as value where the prompt will be run in inline chat instead 😀

Wow. This is ace!

abeatrix · 2023-09-20T07:57:28Z

The current one is the one from the old recipe so I didn't really do any prompt engineering on this, but yours looks good to me! @toolmantim

edit: we can make the changes to the prompt in a different PR

philipp-spiess

Awesome stuff! I didn't review everything but got curious how the smart selection works so I left some comments on that 👀

vscode/src/editor/utils.ts

philipp-spiess · 2023-09-20T16:44:38Z

vscode/src/editor/utils.ts

+    const symbols = await getSymbolRanges(uri).then(r => r.filter(s => s.kind === vscode.SymbolKind.Class))
+    // Get the folding ranges for the document,
+    const ranges = await getFoldingRange(uri).then(r => r.filter(r => r.kind !== 2 && r.kind !== 1 && r.kind !== 3))
+    // remove ranges that match the symbols from ranges


Can you explain why? It's not immediatly clear to me

IT's coz if there is a function inside a class then the whole class is taken rather than the function alone.

1 | class OuterClass { 2 | // ... 3 | function something { 4 | // ... cursor 5 | } 6 | // ... 7 | }

You wanna get line 3-5 instead of 1-7. But you get 1-7 unless you exclude folding ranges that match class symbol.

What @arafatkatze said! I also just added a comment to the function to detail my thought process, here is what I added:

The purpose of filtering to keep only folding ranges that contain other folding ranges is to find the outermost folding range enclosing the cursor position.

Folding ranges can be nested - you may have a folding range for a function that contains folding ranges for inner code blocks.

By filtering to ranges that contain other ranges, it removes the inner nested ranges and keeps only the outermost parent ranges.

This way when it checks for the range containing the cursor, it will return the outer range that fully encloses the cursor location, rather than an inner range that may only partially cover the cursor line.

However, if we have kept the ranges for classes, this may only return ranges for classes that contain individual methods rather than the outermost range of the methods within a class.

OH this is only class symbols! This wasn't clear to me when I read it. So basically you just want to get all folding ranges that aren't classes, that makes sense.

But can't nesting of folding ranges happen regardless? Like if you have a function inside a function:

function createFoo() { return { bar() { ....}, baz() { ....}, } }

Maybe we should find nesting and prefer the leaves?

Oh, it looks like the prompt diff from #1116 (review) wasn't included in the end @abeatrix?

I didn't make any changes to the prompt in that PR since that was for the /doc behavior change.

Do you want to create a new PR with your change for the prompt?

@toolmantim Phillips was referring to the changes suggested by @arafatkatze i believe. We are only adding docstring for function scoops (with Smart Selection)

@abeatrix ah! I don't think I've a good enough test suite / cases to verify prompt changes across more examples… I might leave that to you, unless we have a process for it I can follow.

after seeing it with the action, I wonder if we shouldn't make it so that only function folding ranges are considered for these. Especially when the action is "document code" since I would expect docstrings to be added for functions only

I agree with this given that just making doc strings for functions makes the most sense. IT would be still be nice to have some guardrails see this video, so I was able to send 10K tokens for my case and I can prolly send more if I want.
.

WE could probably just have something that says like function context too large for LLM etc for the edge case of very large function. But yeah focussing on the function alone makes perfect sense.

@philipp-spiess sorry for the ping but I wasn't sure if this got lost in the sea of comments.

arafatkatze · 2023-09-20T18:35:07Z

@abeatrix This is truly amazing. Such a genius solution to this perplexing problem.

Sometimes the lines of the symbols ranges and the folding ranges don't match exactly and therefore the symbol range isn't removed.

            if (r.start === symbolRange.start.line && r.end === symbolRange.end.line) {
                ranges.splice(i, 1)
                i--
            }

This check is missed sometimes.

IF you try inside the class in /Users/arafatkhan/Desktop/cody/vscode/src/completions/providers/unstable-fireworks.ts and /Users/arafatkhan/Desktop/cody/vscode/src/completions/providers/unstable-openai.ts you will see that the folding range is missed so the functions inside the class take up the whole class instead of the function alone. Some function to do "close enough" match instead of exact match would help a lot here.

abeatrix · 2023-09-20T22:59:22Z

@toolmantim Updated the PR to fix the Indention issues you were seeing + added autoApply mode to fixup 🎉

autoapply-doc.mov

@arafatkatze thanks for your detailed feedback and for testing out the changes! smartSelection, unfortunately only works for files where users have lang servers running. If not, I think we should just fallback to using the manual selection for now, but if you have any suggestions on how we can extract ranges for functions in those cases please let us know. We can also look into this as a follow-up PR too (happy to collab!) 😄

toolmantim · 2023-09-21T00:13:23Z

@abeatrix just tested this, and it works great! I tested undo/redo as well, and seems to be fine. The indentation it inserts is perfect now too.

All it needs is that prompt update and I think this is ready to rock! 🙌🏼🙌🏼🙌🏼

arafatkatze · 2023-09-21T01:01:19Z

lib/shared/src/chat/prompts/index.ts

@@ -1,6 +1,9 @@
 import * as defaultPrompts from './default-prompts.json'
 import { toSlashCommand } from './utils'

+// A list of default cody commands
+export const defaultCodyCommands = ['ask', 'doc', 'edit', 'smell', 'test', 'reset']


Nitpick: Not used anywhere

toolmantim · 2023-09-21T01:40:39Z

Do we have a way of doing any e2e testing of this (perhaps with a few diff example languages/repos/scenarios)?

abeatrix · 2023-09-21T01:50:23Z

Do we have a way of doing any e2e testing of this (perhaps with a few diff example languages/repos/scenarios)?

Hmmm it's using fixup which is already covered by an e2e test.

Is there something you have in mind that you want to make sure it's covered by a test particularly?

vscode/src/editor/utils.ts

toolmantim · 2023-09-21T02:54:44Z

Sounds like we don’t have this yet, but was just checking if there was a more automated way to run this command with diff scenarios of repos, files and selection… and verify the quality of output and behavior. My testing is still fairly adhoc.

vscode/src/editor/utils.ts

Co-authored-by: Arafat <arafat.da.khan@gmail.com>

csells · 2023-09-21T16:31:34Z

is there a way to keep the new docs commenting consistent with the rest of the doc comments in the file, e.g. // vs /* */

abeatrix · 2023-09-21T16:41:01Z

is there a way to keep the new docs commenting consistent with the rest of the doc comments in the file, e.g. // vs /* */

Yes, that requires some prompt engineering, which we will work on next in another PR! 😄

abeatrix · 2023-09-22T02:08:15Z

@valerybugakov @dominiccooney @philipp-spiess @umpox can I have one of you review this PR when you have the time, please? Multiple people have tested what's in the change already (see conversations above), but will need someone from the eng side to do the final review/approval to move this forward, thank you 🙇‍♀️

philipp-spiess

Good stuff

philipp-spiess · 2023-09-22T13:16:51Z

vscode/src/editor/utils.ts

+ * However, if we keep the ranges for classes, this will then only return ranges for classes that contain individual methods rather
+ * than the outermost range of the methods within a class. So the first step is to remove class ranges.


I think this is not specific to classes btw. In some languages, it might be giant objects or modules. also in JS you could have a constructor function returning an object.

What about we define a target LOC count and while traversing upwards, we filter out ranges that are too large. Would that make sense?

What about we define a target LOC count and while traversing upwards, we filter out ranges that are too large. Would that make sense?

Yea it does that's smarter too and removes the dependency of the user having code intel installed for the language! I can work on refining this in a follow-up PR, thanks for the suggestions!

Does graph context already have something similar in place I can refer to?

No my implementation is much worse and only looks at the document sections (so for classes it's currently useless). I was hoping I can just use your implementation after that 🙈

philipp-spiess · 2023-09-22T13:17:29Z

vscode/src/editor/utils.ts

+        for (const cRange of classRanges) {
+            for (let i = 0; i < foldingRanges.length; i++) {
+                const r = foldingRanges[i]
+                if (Math.abs(r.start - cRange.start.line) <= 1 && Math.abs(r.end - cRange.end.line) <= 1) {


Annoying that folding ranges are not normal ranges and thus have no contains() 🙃

@philipp-spiess omg right?!!! That tricked Cody autocompletion too 🤣

This PR adds the missing Changelog items for [pull/1116 1139](#1116) & #1139 Also fixed a minor oversight from pull/1116. ## Test plan  changelog update

This updates the default prompt definitions from JSON to a regular TS file, so we can better see changes in diffs & review suggestions (e.g. #1116 (review)), and add more documentation for why certain prompt sentences exist. Part of #1178 Side note: custom commands don't get to enjoy this luxury, but we could add `Array` support for the `prompt` key at least. ## Test plan - Ran each command and verified they still worked

abeatrix added 2 commits September 19, 2023 12:06

feat: add smart selection and update /doc command

a7792c2

revert change for explain command

ddba824

abeatrix requested a review from toolmantim September 19, 2023 19:10

abeatrix changed the title ~~feat: add smart selection and update /doc command~~ feat: new /doc command with smart selection Sep 19, 2023

abeatrix added 3 commits September 19, 2023 12:45

Merge branch 'main' into bee/new-doc

e8f3012

remove class range from getCursorFoldingRange

f20fdfe

fix lint

72c4695

abeatrix marked this pull request as ready for review September 20, 2023 06:45

abeatrix requested review from taras-yemets and a team September 20, 2023 06:55

toolmantim reviewed Sep 20, 2023

View reviewed changes

abeatrix added 2 commits September 20, 2023 07:36

add doclangid to commandrunner

0b8ef49

handle doc command range differently for python

e7c5825

abeatrix requested a review from a team September 20, 2023 16:00

philipp-spiess reviewed Sep 20, 2023

View reviewed changes

abeatrix added 2 commits September 20, 2023 14:46

add autoapply+fix indent+replace cody.action.fixup

0fc84df

add docstring and test for utils

356ea6c

arafatkatze reviewed Sep 21, 2023

View reviewed changes

vscode/src/editor/utils.ts Outdated Show resolved Hide resolved

arafatkatze reviewed Sep 21, 2023

View reviewed changes

vscode/src/editor/utils.ts Outdated Show resolved Hide resolved

Update vscode/src/editor/utils.ts

57beee9

Co-authored-by: Arafat <arafat.da.khan@gmail.com>

add types handling

a6e5ba7

abeatrix requested a review from a team September 22, 2023 02:05

Merge branch 'main' into bee/new-doc

c98c381

philipp-spiess approved these changes Sep 22, 2023

View reviewed changes

Extract foldingRange utils into separate functions

25ac25c

abeatrix merged commit 40316a7 into main Sep 22, 2023
11 checks passed

abeatrix deleted the bee/new-doc branch September 22, 2023 14:05

abeatrix mentioned this pull request Sep 22, 2023

add missing changelog for pull/1116 1139 #1147

Merged

arafatkatze mentioned this pull request Sep 25, 2023

refactor: expand code action range #1163

Merged

toolmantim mentioned this pull request Sep 27, 2023

refactor: change default prompts from JSON to Typescript #1197

Merged

toolmantim mentioned this pull request Oct 4, 2023

Add docs for the new custom commands mode property sourcegraph/sourcegraph#57328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new /doc command with smart selection #1116

feat: new /doc command with smart selection #1116

abeatrix commented Sep 19, 2023 •

edited

Loading

toolmantim left a comment •

edited

Loading

toolmantim Sep 20, 2023

abeatrix Sep 20, 2023

toolmantim Sep 20, 2023

abeatrix commented Sep 20, 2023 •

edited

Loading

philipp-spiess left a comment

philipp-spiess Sep 20, 2023

arafatkatze Sep 20, 2023

abeatrix Sep 20, 2023

philipp-spiess Sep 22, 2023

philipp-spiess Sep 22, 2023

abeatrix Sep 26, 2023

abeatrix Sep 26, 2023 •

edited

Loading

toolmantim Sep 26, 2023

arafatkatze Sep 26, 2023

arafatkatze Sep 27, 2023

arafatkatze commented Sep 20, 2023 •

edited

Loading

abeatrix commented Sep 20, 2023 •

edited

Loading

toolmantim commented Sep 21, 2023

arafatkatze Sep 21, 2023

toolmantim commented Sep 21, 2023

abeatrix commented Sep 21, 2023

toolmantim commented Sep 21, 2023

csells commented Sep 21, 2023

abeatrix commented Sep 21, 2023

abeatrix commented Sep 22, 2023 •

edited

Loading

philipp-spiess left a comment

philipp-spiess Sep 22, 2023

abeatrix Sep 22, 2023

philipp-spiess Sep 22, 2023

philipp-spiess Sep 22, 2023

abeatrix Sep 22, 2023

		* However, if we keep the ranges for classes, this will then only return ranges for classes that contain individual methods rather
		* than the outermost range of the methods within a class. So the first step is to remove class ranges.

feat: new /doc command with smart selection #1116

feat: new /doc command with smart selection #1116

Conversation

abeatrix commented Sep 19, 2023 • edited Loading

Follow-up

Test plan

toolmantim left a comment • 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

abeatrix commented Sep 20, 2023 • edited Loading

philipp-spiess 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

abeatrix Sep 26, 2023 • 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

arafatkatze commented Sep 20, 2023 • edited Loading

abeatrix commented Sep 20, 2023 • edited Loading

toolmantim commented Sep 21, 2023

Choose a reason for hiding this comment

toolmantim commented Sep 21, 2023

abeatrix commented Sep 21, 2023

toolmantim commented Sep 21, 2023

csells commented Sep 21, 2023

abeatrix commented Sep 21, 2023

abeatrix commented Sep 22, 2023 • edited Loading

philipp-spiess 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

abeatrix commented Sep 19, 2023 •

edited

Loading

toolmantim left a comment •

edited

Loading

abeatrix commented Sep 20, 2023 •

edited

Loading

abeatrix Sep 26, 2023 •

edited

Loading

arafatkatze commented Sep 20, 2023 •

edited

Loading

abeatrix commented Sep 20, 2023 •

edited

Loading

abeatrix commented Sep 22, 2023 •

edited

Loading