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

move stable globals out of execution loop #649

Merged
merged 4 commits into from
Aug 23, 2024
Merged

move stable globals out of execution loop #649

merged 4 commits into from
Aug 23, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 23, 2024

  • A new file globals.ts is introduced in the core/src package. This file includes several important functions such as installGlobals() and resolveGlobals(). These functions seem to be responsible for setting global variables in different environments (Browser and Node.js) and also, stringifying and parsing different formats: YAML, CSV, INI, XML, JSONL and MD. 🌍

  • The main.ts file in the cli/src package is updated to install these globals by calling installGlobals() from the newly created globals.ts file. 🔧

  • The global objects YAML, INI, CSV, XML, MD, JSONL and AICI are now handled within the installGlobals() function. These used to be individually declared in the promptcontext.ts file, which has now been simplified, enhancing code readability. 📄

  • Cleaning up was carried out in importprompt.ts and promptcontext.ts. Unnecessary import statements and repeated code relating to global object instantiation were eliminated, directing to use the new functions in the globals.ts file. This makes the code leaner and more maintainable. 🔍⚡️

  • User facing changes: The public API in prompt_template.d.ts has been simplified by removing redundant entries for several global variables (YAML, XML, JSONL, CSV, INI, AICI, MD). This will likely result in a cleaner and more streamlined API for the users. 🚀

In essence, these changes seem to be a part of a code refactor aimed at cleaning up redundant code and enhancing maintainability and ease of use.

generated by pr-describe

Copy link

The pull request involves the following major changes:

  1. A new file globals.ts is introduced which initializes global objects such as YAML, CSV, INI, XML, MD, JSONL, and AICI. There is a function installGlobals which adds these global objects to the global environment. Also, in globals.ts, the resolveGlobal function is moved from importprompt.ts.

  2. In the main.ts file, there is an addition of importing and calling the installGlobals function.

  3. There are removals of code blocks from importprompt.ts and promptcontext.ts that were initializing the global objects which are now done by installGlobals in globals.ts.

  4. In prompt_template.d.ts, the global objects were removed from the PromptContext interface.

Overall, the changes suggest a refactor to centralize the management of global objects. These changes improve readability and maintainability by reducing code duplication and centralizing the management of global objects.

However, there are a few concerns:

  1. The installGlobals function modifies the global environment in place which could potentially interfere with other scripts if they also manipulate the same global objects.
  2. It's not clear where the installGlobals function is called. It is imported and used in main.ts, but is it being used everywhere the global objects are required?

Given these, the code changes can be approved but with a recommendation for further review on potential implications on the usage of global variables.

LGTM 🚀

generated by pr-review

@@ -56,7 +57,7 @@ export async function parseTokens(
filesGlobs: string[],
options: { excludedFiles: string[]; model: string }
) {
const { model = "gpt4" } = options || {}
const { model = DEFAULT_MODEL } = options || {}

Choose a reason for hiding this comment

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

The default value for the model option has been changed from "gpt4" to DEFAULT_MODEL. If the DEFAULT_MODEL constant is not defined as "gpt4", this could potentially break existing functionality.

generated by pr-review-commit missing_default

throw new Error("Could not find global")
}
import { pathToFileURL } from "url"
import { resolveGlobal } from "./globals"

Choose a reason for hiding this comment

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

The function resolveGlobal() has been moved to a different file. Make sure that this function is not used elsewhere in the codebase, as this could potentially break functionality.

generated by pr-review-commit import_change

@pelikhan pelikhan merged commit 35a4c28 into main Aug 23, 2024
9 checks passed
@pelikhan pelikhan deleted the globals branch August 23, 2024 22:36
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