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

Update external client check + google #193

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Conversation

roodboi
Copy link
Collaborator

@roodboi roodboi commented Jan 14, 2025

Important

Update dependencies, remove baseURL check in type guard, and add Google client tests.

  • Behavior:
    • Removed baseURL check from isGenericClient() in instructor.ts.
    • Added GEMINI_API_KEY to environment variables in test-pr.yml and test.yml.
  • Dependencies:
    • Updated @anthropic-ai/sdk to 0.33.1, llm-polyglot to 2.5.0, and openai to 4.78.1 in package.json.
    • Updated bun to 1.1.43 in .tool-versions.
  • Testing:
    • Added google.test.ts for testing Google client integration with various scenarios.

This description was created by Ellipsis for 869c0ed. It will automatically update as commits are pushed.

Copy link

changeset-bot bot commented Jan 14, 2025

🦋 Changeset detected

Latest commit: 869c0ed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@instructor-ai/instructor Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to cbfb9af in 29 seconds

More details
  • Looked at 349 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. src/instructor.ts:478
  • Draft comment:
    Removing the 'baseURL' check in the 'isGenericClient' function might lead to incorrect identification of a generic client. Consider verifying if this change aligns with the intended logic.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. src/instructor.ts:475
  • Draft comment:
    Function and method names should follow consistent patterns. Consider renaming isGenericClient to follow the same pattern as other methods.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
3. src/instructor.ts:478
  • Draft comment:
    If library code changes, ensure that documentation is updated to reflect the removal of 'baseURL' check in isGenericClient.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_zLuxbXy6AM941pj5


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

cloudflare-workers-and-pages bot commented Jan 14, 2025

Deploying instructor-js with  Cloudflare Pages  Cloudflare Pages

Latest commit: 869c0ed
Status:⚡️  Build in progress...

View logs

@roodboi roodboi changed the title Add google tests update deps Update external client check + google Jan 14, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on a25005a in 29 seconds

More details
  • Looked at 11 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .changeset/eleven-needles-fry.md:5
  • Draft comment:
    Ensure that the changeset description is clear and matches the actual changes in the code. If the baseUrl check removal affects functionality, it should be documented here.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The changeset file is correctly formatted and provides a clear description of the changes made in the PR.
2. .changeset/eleven-needles-fry.md:1
  • Draft comment:
    Ensure this new markdown file is added to mkdocs.yml.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_6vHQfz2rw6nQQ8Fy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 869c0ed in 19 seconds

More details
  • Looked at 27 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/test-pr.yml:25
  • Draft comment:
    Ensure that the GEMINI_API_KEY secret is correctly set in the repository settings to avoid runtime errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addition of the GEMINI_API_KEY is consistent across both workflow files, ensuring that the secret is available for both PR and main branch tests.
2. .github/workflows/test-pr.yml:25
  • Draft comment:
    Ensure that documentation and tests are updated to reflect the addition of GEMINI_API_KEY. This applies to both test-pr.yml and test.yml.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_XaYxoDfk9Uc2sU40


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@roodboi roodboi merged commit da449de into main Jan 14, 2025
1 of 3 checks passed
@roodboi roodboi deleted the add-google-tests-update-deps branch January 14, 2025 04:32
@roodboi roodboi mentioned this pull request Jan 14, 2025
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.

2 participants