This repository was archived by the owner on Aug 1, 2025. It is now read-only.
fix(chat): document code command now properly indents generated comments especially python docstrings#8039
Open
ichim-david wants to merge 11 commits intomainfrom
Open
fix(chat): document code command now properly indents generated comments especially python docstrings#8039ichim-david wants to merge 11 commits intomainfrom
ichim-david wants to merge 11 commits intomainfrom
Conversation
- Enhanced FixupController to handle indentation correctly when inserting code snippets, especially for Python docstrings. - Adjusted docstring placement logic in `getLanguageSpecificQueryWrappers` to ensure correct positioning above symbols. - Added a special case to handle Python documentation indentation. - Fixed an issue where the LLM returns the entire code block instead of only the docstring. ## Test plan - Tested with various code snippets and docstrings to ensure correct indentation and placement. - Verified that the fix addresses the issue of the LLM returning the entire code block instead of only the docstring.
- Modified the symbol capture condition in `getLanguageSpecificQueryWrappers` to correctly identify symbols based on their position relative to the start position. - Changed `node.startPosition.row <= start.row` to `node.startPosition.row < start.row` to refine the symbol matching logic. This fixes the tests from `vscode/src/tree-sitter/query-tests/documentable-nodes.test.ts`
- Adjusted indentation logic in `FixupController.ts` to handle cases where the target indentation is greater than the text indentation. - Added a special case for Python documentation to avoid incorrect indentation. - Modified `claude.ts` to prevent empty responses from Claude when generating docstrings for Python functions by excluding the function definition line from the stop sequences. - Fixed a bug in `FixupController.ts` where the entire code block was being returned instead of just the docstring for Python document code actions. Now, the code correctly replaces the original range with the generated text.
- With the fixes made to the white space, we get a better spacing distribution than what we had before.
…test coverage (#8080) This commit addresses an issue where `vscode.commands.executeCommand` would throw a TypeError when invoked with null arguments. The fix involves adding a null check before accessing `Symbol.iterator` when handling arguments. Additionally, this commit includes comprehensive test coverage for `vscode.commands.executeCommand`, specifically focusing on handling null, undefined, and iterable arguments correctly. The following changes were made: - Added null check in `vscode-shim.ts` to prevent TypeError when `args[0]` is null. - Added tests in `vscode-shim.test.ts` to verify correct handling of null, undefined, and iterable arguments. - Added no-op handlers for `textEditor/selection` and `textEditor/revealRange` requests in `TestClient.ts` to support testing scenarios. Test plan: - Run `document-code.test.ts` and vscode-shim.test.ts` to ensure all tests pass.
This commit improves the handling of Python docstring insertion within the FixupController. Specifically, it addresses scenarios where the docstring should be inserted on the line following the function or class definition. The changes include: - Introduce `isPythonFile` and `isDocIntent` variables for better readability. - Add a condition to increment the insertion point line by one when inserting docstrings for Python files if the start line text is empty. This ensures the docstring is placed on the next line after the function or class definition. - Update the `editBuilder.insert` call to use the potentially adjusted `insertionPoint`.
umpox
reviewed
Jul 2, 2025
Comment on lines
+983
to
+989
| line.startsWith(' '.repeat(textIndentSize)) | ||
| ) { | ||
| return line.trimStart() | ||
| } | ||
|
|
||
| // Add the calculated indentation difference | ||
| return ' '.repeat(indentDifference) + line |
Contributor
There was a problem hiding this comment.
Does this work on files that use tabs, not spaces, for indentation?
| // Remove any leading whitespace from the first line, as we are inserting at the insertionPoint | ||
| // Keep any trailing whitespace on the last line to preserve the original indentation. | ||
| const replacementText = textLines.join('\n').trimStart() | ||
| // Get the indentation context for the insertion point |
Contributor
There was a problem hiding this comment.
General comment, it would be great to extract this logic into a util and add tests for it
There's quite a few edge cases (python file, doc intent, spaces vs tabs). Would be good to have those tested to avoid any regressions
Amp should help do most of the heavy lifting for the test file!
umpox
reviewed
Jul 2, 2025
Comment on lines
53
to
+55
| /** | ||
| * Prints a greeting message if shouldGreet is true. | ||
| */ | ||
| * Prints a greeting message if shouldGreet is true. | ||
| */ |
Contributor
There was a problem hiding this comment.
This is nice validation that the fix is good!
umpox
approved these changes
Jul 2, 2025
…troller This commit fixes an issue where the FixupController was hardcoding the indentation size for Python docstrings to 4 spaces, regardless of the editor's tab size settings. The change replaces the hardcoded value with a call to `getEditorTabSize()` to ensure that the docstring indentation matches the user's preferred tab size. This improves consistency and code style for Python projects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pr deals with a couple of issues:
Test plan