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

set lsp brige port for ask code command #313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yangbobo2021
Copy link
Contributor

@yangbobo2021 yangbobo2021 commented Sep 20, 2023

PR Type

enhancement


Description

  • Pass LSP bridge port to askcode_index_query.py for more accurate query results.
  • Add abstract method for getting LSP bridge port in UiUtil interface.
  • Implement getLSPBrigePort method in UiUtilWrapper and UiUtilVscode classes.

Changes walkthrough 📝

Relevant files
Enhancement
sendMessage.ts
Pass LSP bridge port to askcode_index_query.py                     

src/handler/sendMessage.ts

  • Added port argument to askCode function call.
  • Updated args in commandRun.spawnAsync call to include port.
  • +3/-1     
    uiUtil.ts
    Add abstract method for getting LSP bridge port                   

    src/util/uiUtil.ts

    • Added getLSPBrigePort abstract method to UiUtil interface.
    +5/-0     
    uiUtilWrapper.ts
    Implement getLSPBrigePort method in UiUtilWrapper               

    src/util/uiUtilWrapper.ts

  • Added implementation of getLSPBrigePort method in UiUtilWrapper class.

  • uiUtil_vscode.ts
    Implement getLSPBrigePort method in UiUtilVscode                 

    src/util/uiUtil_vscode.ts

  • Added implementation of getLSPBrigePort method in UiUtilVscode class.
  • +5/-0     
    askcode_index_query.py
    Pass LSP bridge port to askcode_index_query.py                     

    tools/askcode_index_query.py

    • Added lsp_brige_port parameter to query function.
    +6/-4     

    @runjinz
    Copy link

    runjinz commented Apr 7, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes across multiple files including TypeScript and Python, integrating a new feature that requires understanding of both the extension's architecture and the external command execution. The changes are not overly complex but require careful consideration of error handling, data types, and integration points.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The addition of port to the args array in sendMessage.ts does not check if port is undefined before using it. This could lead to unexpected behavior or errors if getLSPBrigePort() fails to retrieve a port.

    Error Handling: There is no error handling for the asynchronous call to getLSPBrigePort(). If this call fails, it might not be clear to the user what went wrong.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/handler/sendMessage.ts
    suggestion      

    Consider checking if port is undefined before appending it to the args array. This ensures that the command is only run with a valid port. [important]

    relevant lineconst args = [UiUtilWrapper.extensionPath() + "/tools/askcode_index_query.py", "query", message.text, tempFile, port];

    relevant filesrc/util/uiUtil_vscode.ts
    suggestion      

    Implement error handling for the getLSPBrigePort method to gracefully handle cases where the LangBrige.getAddress command fails or returns an unexpected result. [important]

    relevant lineconst port = await vscode.commands.executeCommand('LangBrige.getAddress') as number | undefined;;

    relevant filetools/askcode_index_query.py
    suggestion      

    Validate the lsp_brige_port argument in the query function to ensure it's an integer and within a valid port range. This prevents potential runtime errors. [medium]

    relevant linedef query(question: str, doc_context: str, lsp_brige_port: int):

    relevant filetools/askcode_index_query.py
    suggestion      

    Convert port from string to integer before passing it to the query function to ensure type safety. [medium]

    relevant lineport = sys.argv[4]

    @runjinz
    Copy link

    runjinz commented Apr 7, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Add error handling for when getLSPBrigePort() returns undefined.

    Consider handling the scenario where port might be undefined. This could occur if
    getLSPBrigePort() fails to retrieve a port. You could either provide a default port or
    handle the error more gracefully to avoid runtime issues.

    src/handler/sendMessage.ts [41]

    -const port = await UiUtilWrapper.getLSPBrigePort();
    +const port = await UiUtilWrapper.getLSPBrigePort() || 'defaultPort';
     
    Best practice
    Remove unnecessary semicolon for cleaner code.

    Remove the unnecessary semicolon after the as number | undefined type assertion in the
    getLSPBrigePort method to adhere to TypeScript syntax best practices.

    src/util/uiUtil_vscode.ts [127]

    -const port = await vscode.commands.executeCommand('LangBrige.getAddress') as number | undefined;;
    +const port = await vscode.commands.executeCommand('LangBrige.getAddress') as number | undefined;
     
    Use path.join for constructing file paths for cross-platform compatibility.

    Use path.join for constructing paths to ensure cross-platform compatibility. Direct string
    concatenation for paths might lead to issues on different operating systems.

    src/handler/sendMessage.ts [96]

    -const args = [UiUtilWrapper.extensionPath() + "/tools/askcode_index_query.py", "query", message.text, tempFile, port];
    +const args = [path.join(UiUtilWrapper.extensionPath(), "tools", "askcode_index_query.py"), "query", message.text, tempFile, port];
     
    Bug
    Convert command line argument port to integer before use.

    Ensure the port argument is converted to an integer before passing it to the query
    function, as it is received as a string from the command line arguments.

    tools/askcode_index_query.py [183-184]

    -port = sys.argv[4]
    +port = int(sys.argv[4])
     query(question, doc_context, port)
     
    Enhancement
    Add validation for port argument in command line interface.

    Add error handling for the case where the port argument is missing or not a valid integer.
    This will improve the robustness of the command line interface.

    tools/askcode_index_query.py [177-179]

    -if len(sys.argv) < 5:
    +if len(sys.argv) < 5 or not sys.argv[4].isdigit():
         print("Usage: python index_and_query.py query [question] [doc_context] [port]")
         sys.exit(1)
     

    @runjinz
    Copy link

    runjinz commented May 18, 2024

    PR Description updated to latest commit (8865f28)

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants