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

display and check server version #610

Merged
merged 5 commits into from
Aug 8, 2024
Merged

display and check server version #610

merged 5 commits into from
Aug 8, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 8, 2024

  • Documentation updates 📝: Adjusted some instructions and code snippets for clarity in "docs/src/content/docs/reference/cli/index.mdx". This should make it easier for users to follow along.
  • Dependency changes 🗃️: Removed dependency '@types/semver' and 'semver' from the CLI's package.json and integrated them within the core's package.json. This reduces redundancy.
  • Code restructuring 🏗️: The method for comparing semantic versioning (semver) is now managed centrally in a new file 'packages/core/src/semver.ts'. This creates a more reusable codebase.
  • Codebase enhancements 🛠️: Made adjustments in 'packages/core/src/server/client.ts' and 'vscode/src/servermanager.ts' to handle version checking better, including checking against outdated versions of the command line tool. This should help enforce best practices among users.
  • Build/CI process 🏗️: The CLI tool no longer prompts the user for confirmation before execution. This change should optimize the continuous integration (CI) scenario.

generated by pr-describe


Check that your node version is at least 20._ and npm 10._ by running this command.
- Check that your node version is at least 20._ and npm 10._ by running this command.
Copy link

Choose a reason for hiding this comment

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

Version placeholders are incomplete; please specify the exact versions required.

generated by pr-docs-review-commit version_placeholder


```sh "--yes"
npx --yes genaiscript ...
```

You can also lock this call to a particular version.
- Specify the version range to avoid unexpected behavior with cached installations of the CLI using npx.
Copy link

Choose a reason for hiding this comment

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

The version specification should be more precise to ensure reproducibility.

generated by pr-docs-review-commit version_specification

@@ -166,7 +166,6 @@ export interface ChatChunk extends RequestMessage {

export type RequestMessages =
| ServerKill
| ServerVersion
| ServerEnv
Copy link

Choose a reason for hiding this comment

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

Duplicate import of ServerVersion. This can lead to confusion and unnecessary code. Please remove the duplicate import. 🧹

generated by pr-review-commit duplicate_import

Copy link

github-actions bot commented Aug 8, 2024

The changes in the GIT_DIFF involve the following:

  1. The semverSatisfies function import reference was moved from packages/cli/src/cli.ts to a new file packages/core/src/semver.ts. This new file also introduces a new function semverParse which is not in use in this diff.

  2. The ServerResponse type was added to the import statement in packages/core/src/server/client.ts. It also changes the return type of the version() method from string to ServerResponse.

  3. The ServerVersion type was removed from packages/core/src/server/messages.ts.

  4. The file packages/vscode/src/servermanager.ts now imports semverParse and semverSatisfies from packages/core/src/semver.ts. There's also a new version check added to the event listener for OPEN events. If the version check fails, a warning message is displayed.

There's no clear functional issue, but some observations can be made:

  • It's not clear why the ServerVersion type was removed from packages/core/src/server/messages.ts. This could impact any code that relied on it.

  • The semverParse function is not being used in this diff.

  • There are changes to the version() method in packages/core/src/server/client.ts which could impact existing code if it expects a string return type.

Without more context, it's difficult to flag specific concerns. However, these changes do not seem to introduce any major functional issues. Accordingly, my response would be:

"LGTM 🚀"

generated by pr-review

@pelikhan pelikhan merged commit a8fc1e6 into main Aug 8, 2024
7 of 9 checks passed
@pelikhan pelikhan deleted the serverversion branch August 8, 2024 12:32
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