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

fix: doc prompt improvements #1198

Merged
merged 16 commits into from
Oct 3, 2023
Merged

fix: doc prompt improvements #1198

merged 16 commits into from
Oct 3, 2023

Conversation

toolmantim
Copy link
Contributor

@toolmantim toolmantim commented Sep 27, 2023

This updates the /doc prompt to product better quality documentation across more cases. It:

  • Reduces the chance that parameters are documented unnecessarily (e.g. for code that doesn't take parameters)
  • Reduces the verbosity of documentation, creating more focused explanations
  • Increases the chance that the correct documentation style is used for different parts of the file

Closes #1178

Test plan

  • Cloned each example repo, created before + after branches
  • Ran /doc over each example, committing main to before and this branch to after
  • Git diffed branches

@toolmantim
Copy link
Contributor Author

toolmantim commented Sep 27, 2023

/doc test case diffs between 5291932 (main) and 7cc624e7a7cede4f4bb9ef7dfda1a8cac76e32d2:

sourcegraph/sourcegraph/client/wildcard/src/hooks/useKeyboard.ts

Diff
diff --git a/client/wildcard/src/hooks/useKeyboard.ts b/client/wildcard/src/hooks/useKeyboard.ts
index 47e420d8df5..1d6b698e88e 100644
--- a/client/wildcard/src/hooks/useKeyboard.ts
+++ b/client/wildcard/src/hooks/useKeyboard.ts
@@ -8,39 +8,24 @@ interface UseKeyboardProps {
 }
 
 /**
- * Hook to listen for keyboard events.
+ * Hook to listen for keyboard events for specified keys.
  * 
- * @param props - Options for keyboard listening:
- * - detectKeys: Array of key strings or key codes to detect.
- * - keyevent: Event to listen for - 'keyup' or 'keydown'.
- * 
- * @param callback - Callback when detected key is pressed.
+ * @param props - Options for keyboard listening.
+ * @param callback - Handler function called on key event.
  */
 export function useKeyboard(props: UseKeyboardProps, callback: (event: Event) => void): void {
     const { keyevent = 'keyup', detectKeys } = props
     /**
-     * Keys to detect keyboard events for.
-     * Computed with useDeepMemo to avoid unnecessary recomputations.
-     */
+     * Memoize the detectKeys array using useDeepMemo hook.
+     * This ensures keys array is not recreated on every render.
+    */
     const keys = useDeepMemo(detectKeys)
 
     /**
-     * Adds a keyboard event listener.
-     * 
-     * @param callback - Callback function to call when a detected key is pressed.
-     * @param keyevent - Event to listen for - 'keyup' or 'keydown'.
-     * @param keys - Array of key strings or key codes to detect.
-     * 
-     * Inside the callback:
-     * - Checks if the pressed key is in the keys array. 
-     * - If so, calls the callback with the keyboard event.
-     * 
-     * On mount:
-     * - Adds event listener for the specified keyevent.
-     * 
-     * On unmount:
-     * - Removes the event listener.
-     */
+     * Adds keyboard event listener for specified keys. 
+     * Calls callback function when key event occurs.
+     * Removes listener on unmount.
+    */
     useEffect(() => {
         const handleEvent = (event: KeyboardEvent): void => {
             if (keys.includes(event.key)) {

https://github.com/sourcegraph/wizard/blob/2414828d62f7ababd24a2532708e7e94fb5d92bb/pages/index.js

diff --git a/pages/index.js b/pages/index.js
index 3c3a022..8b943c3 100644
--- a/pages/index.js
+++ b/pages/index.js
@@ -2,21 +2,23 @@ import { useCallback, useState, useEffect } from 'react';
 import Head from 'next/head';
 
 /**
- * Home page component.
+ * Home component that renders the setup wizard UI.
  * 
- * @param {string} hostname - The hostname of the Sourcegraph instance.
- * @param {number} port - The port of the Sourcegraph instance. 
- * @param {string} size - The size of the Sourcegraph instance (XS, S, M, L, XL). 
- * @param {string} mode - The launch mode (new, upgrade). 
- * @param {string} version - The version number to upgrade to.
- * @param {string} fileName - The name of the uploaded file. 
- * @param {File} blob - The uploaded file content.
- * @param {boolean} submitted - Whether the launch button has been clicked.
- * @param {Object} uploadStatus - The status of file uploads.
- * @param {string} showErrors - Any errors to display.
- * @param {string} currentStatus - The current status of the launch process.
+ * Manages state for the wizard including:
+ * - Instance size
+ * - Launch mode (new or upgrade) 
+ * - Version number (for upgrades)
+ * - Uploaded SSH key files
+ * - Launch request submitted state
+ * - Current launch status
  * 
- * Renders the setup wizard UI and handles launching/upgrading the instance.
+ * Allows uploading SSH key files for code hosts.
+ * Sends API requests to launch or upgrade instance.
+ * Polls API to wait for instance to be ready, 
+ * then redirects to instance URL.
+ * 
+ * Renders different UI based on state - initial config form,
+ * loading state after launch, and errors if applicable.
  */
 export default function Home() {
   const [hostname, setHostname] = useState(null);
@@ -51,20 +53,8 @@ export default function Home() {
     };
   }
 
-  /**
-   * useEffect hook to handle uploading file when blob and fileName change
-   * 
-   * @param {Object} blob - File object representing uploaded file 
-   * @param {string} fileName - Name of uploaded file
-   * @param {string} hostname - Hostname for API endpoint  
-   * @param {number} port - Port number for API endpoint
-   * 
-   * 1. Set hostname and port based on window.location if not defined
-   * 2. Create FormData object and append blob and fileName
-   * 3. Make POST request to /api/upload endpoint with FormData
-   * 4. On response, update uploadStatus state with result
-   * 5. On error, update showErrors and uploadStatus states
-  */
+  // Uploads file to server if blob and fileName are set. 
+  // Sets upload status and handles errors.
   useEffect(() => {
     if (!hostname) {
       const uri = new URL(window.location.origin);
@@ -129,12 +119,9 @@ export default function Home() {
       const postRequest = makeRequest('POST', JSON.stringify(requestBody));
       /**
        * Checks if the frontend is ready by polling the /api/check endpoint.
-       * 
-       * @param {number} tries - The number of tries remaining to poll the endpoint.
-       * Polls the endpoint every 10 seconds until frontend is ready or tries reaches 0.
-       * 
-       * Calls teardownWizard() if frontend is ready.
-       * Recursively calls checkFrontend() with decremented tries if not ready yet.
+       * Retries up to the given number of tries with a 10 second delay between each try.
+       * If frontend responds as ready, calls teardownWizard().
+       * Otherwise continues polling until tries are exhausted.
        */
       function checkFrontend(tries) {
         if (tries > 0) {
@@ -163,15 +150,8 @@ export default function Home() {
         fetch(`${hostname}/api/${mode}`, postRequest)
           .then((res) => {
             setCurrentStatus('Instance has been launched...');
-            /**
-             * Checks if the frontend is ready by calling the /api/check endpoint. 
-             * 
-             * @param tries - The number of tries remaining to check if the frontend is ready.
-             * Decrements on each try.
-             * 
-             * Calls teardownWizard() if frontend is ready.
-             * Recursively calls checkFrontend() with decremented tries if not ready yet.
-             */
+            // Polls the frontend API endpoint to check if the frontend is ready after launching the instance. 
+            // Retries up to the specified number of tries.
             checkFrontend(20);
             setSubmitted(true);
             const responseText = res.toString();

Base automatically changed from tl/update-default-prompt-format to main September 27, 2023 09:36
Comment on lines 21 to 27
* - Golang
* - TODO
* - Java
* - TODO
* - Python
* - TODO
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abeatrix do you have some example repos you've been using I could add to here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@toolmantim
Copy link
Contributor Author

toolmantim commented Sep 28, 2023

TS example diff:

diff --git a/client/wildcard/src/hooks/useKeyboard.ts b/client/wildcard/src/hooks/useKeyboard.ts
index 47e420d8df5..a3f6772c020 100644
--- a/client/wildcard/src/hooks/useKeyboard.ts
+++ b/client/wildcard/src/hooks/useKeyboard.ts
@@ -8,38 +8,24 @@ interface UseKeyboardProps {
 }
 
 /**
- * Hook to listen for keyboard events.
+ * Hook to call a callback on keyboard events for specified keys.
  * 
- * @param props - Options for keyboard listening:
- * - detectKeys: Array of key strings or key codes to detect.
- * - keyevent: Event to listen for - 'keyup' or 'keydown'.
- * 
- * @param callback - Callback when detected key is pressed.
+ * @param props - Options for key detection.
+ * @param callback - Callback to call on key event.
  */
 export function useKeyboard(props: UseKeyboardProps, callback: (event: Event) => void): void {
     const { keyevent = 'keyup', detectKeys } = props
     /**
-     * Keys to detect keyboard events for.
-     * Computed with useDeepMemo to avoid unnecessary recomputations.
+     * Memoize the detectKeys array using useDeepMemo.
+     * This ensures detectKeys is stable between re-renders, avoiding unnecessary effect cleanup/re-creation.
      */
     const keys = useDeepMemo(detectKeys)
 
     /**
-     * Adds a keyboard event listener.
-     * 
-     * @param callback - Callback function to call when a detected key is pressed.
-     * @param keyevent - Event to listen for - 'keyup' or 'keydown'.
-     * @param keys - Array of key strings or key codes to detect.
-     * 
-     * Inside the callback:
-     * - Checks if the pressed key is in the keys array. 
-     * - If so, calls the callback with the keyboard event.
-     * 
-     * On mount:
-     * - Adds event listener for the specified keyevent.
-     * 
-     * On unmount:
-     * - Removes the event listener.
+     * Adds keyboard event listener on mount 
+     * and removes it on unmount.
+     * Checks if pressed key is in detectKeys array
+     * and calls callback if so.
      */
     useEffect(() => {
         const handleEvent = (event: KeyboardEvent): void => {

JS example diff:

diff --git a/pages/index.js b/pages/index.js
index 3c3a022..fdc25c9 100644
--- a/pages/index.js
+++ b/pages/index.js
@@ -2,21 +2,13 @@ import { useCallback, useState, useEffect } from 'react';
 import Head from 'next/head';
 
 /**
- * Home page component.
+ * Home page component for Sourcegraph setup wizard.
  * 
- * @param {string} hostname - The hostname of the Sourcegraph instance.
- * @param {number} port - The port of the Sourcegraph instance. 
- * @param {string} size - The size of the Sourcegraph instance (XS, S, M, L, XL). 
- * @param {string} mode - The launch mode (new, upgrade). 
- * @param {string} version - The version number to upgrade to.
- * @param {string} fileName - The name of the uploaded file. 
- * @param {File} blob - The uploaded file content.
- * @param {boolean} submitted - Whether the launch button has been clicked.
- * @param {Object} uploadStatus - The status of file uploads.
- * @param {string} showErrors - Any errors to display.
- * @param {string} currentStatus - The current status of the launch process.
+ * Manages state and UI for configuring a new or upgraded instance.
+ * Allows uploading SSH keys. Sends API requests to launch instance.
+ * Polls API and waits for instance to be ready before redirecting.
  * 
- * Renders the setup wizard UI and handles launching/upgrading the instance.
+ * Renders config form, loading state, errors.
  */
 export default function Home() {
   const [hostname, setHostname] = useState(null);
@@ -52,19 +44,15 @@ export default function Home() {
   }
 
   /**
-   * useEffect hook to handle uploading file when blob and fileName change
-   * 
-   * @param {Object} blob - File object representing uploaded file 
-   * @param {string} fileName - Name of uploaded file
-   * @param {string} hostname - Hostname for API endpoint  
-   * @param {number} port - Port number for API endpoint
-   * 
-   * 1. Set hostname and port based on window.location if not defined
-   * 2. Create FormData object and append blob and fileName
-   * 3. Make POST request to /api/upload endpoint with FormData
-   * 4. On response, update uploadStatus state with result
-   * 5. On error, update showErrors and uploadStatus states
-  */
+   * useEffect hook to upload file when blob and fileName are set.
+   * Checks if hostname is set, otherwise gets it from window.location.
+   * Sets up upload status to "Uploading" for fileName.
+   * Creates FormData with blob and fileName to POST.
+   * Tries fetch POST request to /api/upload endpoint.
+   * On success, logs response and sets upload status to response body. 
+   * On error, sets error state and upload status to "Failed".
+   * Depends on blob, fileName, hostname and port.
+   */
   useEffect(() => {
     if (!hostname) {
       const uri = new URL(window.location.origin);
@@ -128,13 +116,10 @@ export default function Home() {
       };
       const postRequest = makeRequest('POST', JSON.stringify(requestBody));
       /**
-       * Checks if the frontend is ready by polling the /api/check endpoint.
-       * 
-       * @param {number} tries - The number of tries remaining to poll the endpoint.
-       * Polls the endpoint every 10 seconds until frontend is ready or tries reaches 0.
-       * 
-       * Calls teardownWizard() if frontend is ready.
-       * Recursively calls checkFrontend() with decremented tries if not ready yet.
+       * Polls the frontend API endpoint to check if the frontend is ready after launching the instance.
+       * Retries up to the given number of tries with a 10 second delay between each try.
+       * If frontend responds as ready, calls teardownWizard().
+       * Otherwise continues polling until tries are exhausted.
        */
       function checkFrontend(tries) {
         if (tries > 0) {
@@ -164,13 +149,15 @@ export default function Home() {
           .then((res) => {
             setCurrentStatus('Instance has been launched...');
             /**
-             * Checks if the frontend is ready by calling the /api/check endpoint. 
+             * Calls checkFrontend() recursively to poll the /api/check endpoint 
+             * up to the given number of tries.
              * 
-             * @param tries - The number of tries remaining to check if the frontend is ready.
-             * Decrements on each try.
+             * checkFrontend() sends a request to /api/check to see if the frontend is ready.
+             * If it gets a 'Ready' response, it calls teardownWizard().
+             * If not, it waits 10 seconds and retries, decrementing tries.
              * 
-             * Calls teardownWizard() if frontend is ready.
-             * Recursively calls checkFrontend() with decremented tries if not ready yet.
+             * This allows polling /api/check until the frontend is ready or we 
+             * run out of tries.
              */
             checkFrontend(20);
             setSubmitted(true);

Golang example diff:

diff --git a/cmd/src/batch_common.go b/cmd/src/batch_common.go
index 98886ba..535c075 100644
--- a/cmd/src/batch_common.go
+++ b/cmd/src/batch_common.go
@@ -410,15 +410,11 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error
 		execUI.DeterminingWorkspacesSuccess(len(workspaces), len(repos), nil, nil)
 	}
 
- // NewArchiveRegistry creates a new repozip.ArchiveRegistry to manage cached repository archives.
+ // archiveRegistry creates a registry for caching repository archives.
  // 
- // opts.client is the src CLI API client.
+ // logManager manages logs for the batch change execution.
  // 
- // opts.flags.cacheDir is the directory on disk where repository archives will be cached.
- // 
- // opts.flags.cleanArchives controls whether existing archives will be deleted and recreated or reused.
- //
- // Returns a configured repozip.ArchiveRegistry instance.
+ // coord coordinates batch change execution across repositories, caching results.
 	archiveRegistry := repozip.NewArchiveRegistry(opts.client, opts.flags.cacheDir, opts.flags.cleanArchives)
 	logManager := log.NewDiskManager(opts.flags.tempDir, opts.flags.keepLogs)
 	coord := executor.NewCoordinator(
@@ -527,23 +523,12 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error
 		execUI.NoChangesetSpecs()
 	}
 
- ```go
- // CreateBatchSpec creates a new batch spec on the backend.
- // It takes in:
- // - ctx: The context
- // - namespaceID: The ID of the namespace to create the batch spec in 
- // - rawSpec: The raw JSON spec for the batch spec
- // - ids: The IDs of the changeset specs to attach to this batch spec
- //
- // It returns:
- // - id: The ID of the created batch spec
- // - url: The URL endpoint for the new batch spec 
- // - err: Any error that occurred
- //
- // It calls execUI.CreatingBatchSpec to indicate it is creating the spec.
- // If there is an error, it calls execUI.CreatingBatchSpecError to display the error.
- // Otherwise, it calls execUI.CreatingBatchSpecSuccess with the preview URL for the new spec.
- ```
+ // CreatingBatchSpec creates a new batch spec on the Sourcegraph
+ // instance with the given namespace ID, raw spec YAML, and changeset
+ // spec IDs. It returns the new batch spec's ID, URL, and any error.
+ // The previewURL is constructed from the returned URL and endpoint config.
+ // If there is an error creating the batch spec, it displays the error
+ // using the execUI.
 	execUI.CreatingBatchSpec()
 	id, url, err := svc.CreateBatchSpec(ctx, namespace.ID, rawSpec, ids)
 	if err != nil {
@@ -654,12 +639,9 @@ func checkExecutable(cmd string, args ...string) error {
 	return nil
 }
 
-/**
-* contextCancelOnInterrupt creates a new context derived from the given parent context that will be canceled when an interrupt signal is received.
-*
-* @param parent The parent context 
-* @return A tuple containing the new derived context and a cancel function to cancel it manually
-*/
+// contextCancelOnInterrupt creates a new context derived from the given parent 
+// context that will be canceled when an interrupt signal is received. It returns 
+// the derived context and a cancel function.
 func contextCancelOnInterrupt(parent context.Context) (context.Context, func()) {
 	ctx, ctxCancel := context.WithCancel(parent)
 	c := make(chan os.Signal, 1)

Java example diff:

diff --git a/com.microsoft.java.lsif.core/src/com/microsoft/java/lsif/core/internal/protocol/Document.java b/com.microsoft.java.lsif.core/src/com/microsoft/java/lsif/core/internal/protocol/Document.java
index 800c66d..3c7f8e3 100644
--- a/com.microsoft.java.lsif.core/src/com/microsoft/java/lsif/core/internal/protocol/Document.java
+++ b/com.microsoft.java.lsif.core/src/com/microsoft/java/lsif/core/internal/protocol/Document.java
@@ -14,18 +14,9 @@ package com.microsoft.java.lsif.core.internal.protocol;
 import com.microsoft.java.lsif.core.internal.IConstant;
 
 /**
- * Document class that extends Vertex.
- * 
- * Represents a document in the LSIF protocol.
- * 
- * Contains the following fields:
- * - uri: The URI of the document.
- * - languageId: The language ID of the document.
- * - contents: The contents of the document.
- * 
- * The constructor takes an ID and URI to initialize the document.
- * 
- * Provides getter and setter methods for the fields.
+ * Represents a source code document.
+ * Extends Vertex to represent a node in the LSIF graph.
+ * Contains the document URI, language ID, and contents.
  */
 public class Document extends Vertex {
 
@@ -36,17 +27,15 @@ public class Document extends Vertex {
 	private String contents;
 
  /**
-  * Constructs a new Document with the given ID and URI.
-  *
-  * @param id The ID of the document.
-  * @param uri The URI of the document.
+  * Constructs a new Document vertex with the given ID and URI.
+  * 
+  * @param id The unique identifier for this document.
+  * @param uri The URI of this document.
   */
 	public Document(String id, String uri) {
   /**
-   * Constructs a new Document with the given ID and type.
-   *
-   * @param id The ID of the document.
-   * @param type The type of the vertex, which should be Vertex.DOCUMENT for a document.
+   * Calls the superclass constructor with the vertex ID and type.
+   * DOCUMENT indicates this vertex represents a source code document.
    */
 		super(id, Vertex.DOCUMENT);
 		this.uri = uri;
@@ -66,9 +55,9 @@ public class Document extends Vertex {
 	}
 
  /**
-  * Sets the contents of the document.
-  *
-  * @param contents The new contents of the document.
+  * Sets the contents of this document.
+  * 
+  * @param contents The new contents of this document.
   */
 	public void setContents(String contents) {
 		this.contents = contents;

Scala example diff:

diff --git a/scip-java/src/main/scala/com/sourcegraph/io/AutoDeletedFile.scala b/scip-java/src/main/scala/com/sourcegraph/io/AutoDeletedFile.scala
index 6005f51..4cfba0b 100644
--- a/scip-java/src/main/scala/com/sourcegraph/io/AutoDeletedFile.scala
+++ b/scip-java/src/main/scala/com/sourcegraph/io/AutoDeletedFile.scala
@@ -8,10 +8,10 @@ import java.nio.file.StandardOpenOption
 import scala.util.Using.Releasable
 
 /**
- * AutoDeletedFile class
- *
- * @param path The path to the file
- * @param oldContent Optional byte array containing the previous content of the file
+ * AutoDeletedFile is a utility class to temporarily write content to a file that gets deleted when this instance goes out of scope.
+ * 
+ * It takes a file path and new content to write. The previous file content is saved and restored when this instance is released.
+ * Useful for writing temporary files in tests.
  */
 class AutoDeletedFile private (
     val path: Path,
@@ -19,11 +19,13 @@ class AutoDeletedFile private (
 )
 
 /**
- * Creates a new AutoDeletedFile instance from the given path and content.
+ * Companion object for AutoDeletedFile that provides factory methods.
+ * 
+ * fromPath creates a new AutoDeletedFile, writing the given newContent to the path.
+ * It saves the existing file content to restore later.
  * 
- * @param path The path to the file
- * @param newContent The new content to write to the file
- * @return A new AutoDeletedFile instance
+ * An implicit Releasable is provided to restore the original file content when the 
+ * AutoDeletedFile goes out of scope.
  */
 object AutoDeletedFile {
   def fromPath(path: Path, newContent: String): AutoDeletedFile = {
@@ -34,12 +36,8 @@ object AutoDeletedFile {
         None
     Files.createDirectories(path.getParent)
     /**
-     * Writes the given byte array to the specified path, creating 
-     * the file if needed and truncating any existing content.
-     *
-     * @param path The path to write to.
-     * @param bytes The byte array to write.
-     * @param options The open options to use, must include CREATE and TRUNCATE_EXISTING.
+     * Writes the given newContent bytes to the path, 
+     * creating/truncating the file as needed.
      */
     Files.write(
       path,

Python example diff:

diff --git a/libs/langchain/langchain/adapters/openai.py b/libs/langchain/langchain/adapters/openai.py
index c91de7aff..0dd9f5909 100644
--- a/libs/langchain/langchain/adapters/openai.py
+++ b/libs/langchain/langchain/adapters/openai.py
@@ -39,15 +39,14 @@ async def aenumerate(
 
 
 def convert_dict_to_message(_dict: Mapping[str, Any]) -> BaseMessage:
-    """
-    Convert a dictionary representing a message from the OpenAI API 
+    """Converts a dictionary representing a message from the OpenAI API 
     to a BaseMessage object.
     
-    Parameters:
-        _dict (Mapping[str, Any]): Dictionary representing a message.
+    Args:
+        _dict: The dictionary representing the message.
     
     Returns:
-        BaseMessage: Message object based on the role in the dictionary.
+        The corresponding BaseMessage object.
     """
     role = _dict["role"]
     if role == "user":
@@ -63,12 +62,14 @@ def convert_dict_to_message(_dict: Mapping[str, Any]) -> BaseMessage:
         return AIMessage(content=content, additional_kwargs=additional_kwargs)
     elif role == "system":
         return SystemMessage(content=_dict["content"])
-    /**
-     * Constructs a SystemMessage object from a dictionary representing a system message.
-     * 
-     * @param _dict The input dictionary containing the system message content under the "content" key.
-     * @return A SystemMessage instance with the content set to the value of the "content" field in the input dictionary.
-     */
+    """Converts an OpenAI API message dict with role "system" to a SystemMessage.
+    
+    Args:
+        _dict: The OpenAI API message dict.
+    
+    Returns:
+        The corresponding SystemMessage.
+    """
     elif role == "function":
         return FunctionMessage(content=_dict["content"], name=_dict["name"])
     else:

@toolmantim toolmantim changed the title fix: update doc prompt to better handle implementation comments fix: doc prompt improvements Sep 28, 2023
@toolmantim toolmantim marked this pull request as ready for review September 28, 2023 13:56
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on improving the prompt!

the prompt seems to works the same on my side as you have shown in your examples, and they look fine with me, but want to check with @dominiccooney @umpox @valerybugakov @philipp-spiess to see if the results look good to them as well, as I am not sure what is the preferred output when users trying to add a docstring (with params or not etc as i see we use param in some part of our repos ).

PS: Since we moved back to using the JSON file, we will need to move the prompt back to that file 🙇‍♀️

Write a brief documentation comment for the selected code. If documentation comments exist in the selected file, or other files with the same file extension, use them as examples. Pay attention to the scope of the selected code (e.g. exported function/API vs implementation detail in a function), and use the idiomatic style for that type of code scope. Only generate the documentation for the selected code, do not include any code. Do not output any other code or comments besides the documentation.

@toolmantim
Copy link
Contributor Author

toolmantim commented Sep 29, 2023

@abeatrix thanks! Will merge main and update.

With the prompt changes, the only place that params should have been removed were for line/impl comments where they don't really belong. If not, let me know.

Another note: I've avoided any language specific instructions and kept it as generic and universal as possible. But in testing other variants we could improve Typescript to consistently use // for impl comments if we added a specific hint for Typescript.

I can't help but shake the feeling that adding language specific hints is inevitable for the highest quality output. To support that, maybe we'd need to be able to append/adjust the prompt string based on file glob pattern?

@toolmantim
Copy link
Contributor Author

Alright, this is in sync with main. I wasn't sure where the docs and test cases should live, so left them out for now.

@toolmantim
Copy link
Contributor Author

It looks like #1163 included a prompt rewrite for /doc too. This PR will need a rewrite + retest before it's ready to merge.

@toolmantim toolmantim marked this pull request as draft October 2, 2023 03:34
@toolmantim toolmantim self-assigned this Oct 2, 2023
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Since your prompt is tested, i think we can replace the one in main with yours.

@abeatrix abeatrix mentioned this pull request Oct 2, 2023
@toolmantim toolmantim marked this pull request as ready for review October 2, 2023 23:49
philipp-spiess pushed a commit that referenced this pull request Oct 3, 2023
I noticed the `/doc` command built from the current main branch has the
following issues:
- a new empty line is added before the 
- if you run the /doc command and then reload VS code, your chat will
start with the output of your last /doc command in chat view


![image](https://github.com/sourcegraph/cody/assets/68532117/0a22af6e-34b1-4663-9e7d-a94e39e32aff)


This PR fixes the issues listed above.

Also merged the prompt with the one tested by Tim in
#1198

IMPORTANT: This PR depends on the prompt improvement made in
#1198

## Test plan

- highlight a function or method in the editor
- right click on the highlighted code, then select `Cody>Document Code`
- Expected behavior: Docstring added directly above the highlighted code
without an extra new line added at the end

<!-- Required. See
https://docs.sourcegraph.com/dev/background-information/testing_principles.
-->

#### Before


https://www.loom.com/share/c1d316d9031d4550952da81d5e5c61fd?sid=3c06e825-e6c3-4c43-a17b-e93f186947a3


https://github.com/sourcegraph/cody/assets/68532117/664cc0b1-c2e2-4e1c-a164-b8b4f8a6d6b5

#### After


https://www.loom.com/share/accef38b7f0f4d0c8e1ba8b89581acd2?sid=29fe90a4-9f5e-4a51-8c40-f4adad1c848f


https://github.com/sourcegraph/cody/assets/68532117/8efddda8-5664-4bcb-adce-732869911184

---------

Co-authored-by: Tim Lucas <t@toolmantim.com>
Co-authored-by: Dominic Cooney <dominic.cooney@sourcegraph.com>
@philipp-spiess
Copy link
Contributor

@abeatrix @toolmantim Do either of you mind fixing the merge conflict? I don't feel like I have the context :)

@philipp-spiess philipp-spiess merged commit 6c5a0e7 into main Oct 3, 2023
12 checks passed
@philipp-spiess philipp-spiess deleted the tl/update-doc-prompt branch October 3, 2023 11:53
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.

Improve /doc prompt for documenting non-function lines
3 participants