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

Refactor fetchText into global function and remove from PromptContext #699

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 9, 2024

This pull request refactors the fetchText function by moving it into a global function and removing it from the PromptContext interface. This change improves code organization and removes unnecessary duplication.

Based on the GIT_DIFF provided, here's a high-level summary of the changes:

  • The fetchText method has been moved from PromptContext class in promptcontext.ts and inserted into installGlobals function in globals.ts file. The purpose is likely to make fetchText a part of backend global context which could potentially make this function globally accessible rather than having it limited to the PromptContext scope.
  • fetchText function's code shows that this function fetches the text content of a file specified by URL or path. It also handles http and local workspace paths, fetching respective contents, encapsulating all related error handling in itself.
  • The structure of fetchText remains the same in the new location. That is, it still takes in urlOrFile and optional fetchOptions as arguments, and returns an object containing information about the operation success status, HTTP status, text content of the file, or the WorkspaceFile object.
  • The removal of fetchText function in PromptContext in promptcontext.ts also resulted in removal of respective imports including fetch and readText, as these were only used by the removed fetchText function.
  • The PromptContext API surface, which is user-facing, has been altered due to the removal of the fetchtext method from it. This change is reflected in the prompt_template.d.ts.

These changes to the codebase do not alter the import statements, which suggests the dependencies of the project remain the same.

We can assume these modifications are aimed at enhancing the project architecture by repositioning functionality to its more logical place and changing the public API as a result of that. 🏗️💻🔄🌐

generated by pr-describe

Copy link

github-actions bot commented Sep 9, 2024

The GIT_DIFF shows changes made to three TypeScript files - "globals.ts", "promptcontext.ts", and "prompt_template.d.ts". The changes are mainly related to handling fetch operations and restructuring of code.

  1. In "globals.ts", a new function fetchText is introduced that handles fetching data from a URL or reading data from a workspace file. It also handles status and error codes.

  2. In "promptcontext.ts", the similar fetchText function was removed. It seems like this function was moved to "globals.ts" to make it available globally to the application rather relying on it on a per prompt context basis.

  3. In "prompt_template.d.ts", the fetchText method signature has been removed. It aligns with the removal of the method from the "promptcontext.ts".

Overall, this seems like a good refactor to make the fetch function more globally accessible in the application. However, potential issues could arise if anything is currently depending on the context-specific implementations of the fetch function in "promptcontext.ts". Also, the removal of the fetchText function might break the existing code which uses this function.

LGTM 🚀 However, make sure to test thoroughly if these changes affect any existing implementations in the system.

generated by pr-review

@pelikhan pelikhan merged commit fba3d0e into main Sep 9, 2024
10 of 11 checks passed
@pelikhan pelikhan deleted the fetch_text_global branch September 9, 2024 12:48
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.

1 participant