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

fix(go): remove unnecessary nil check for lookup model #823

Conversation

yukinagae
Copy link
Contributor

@yukinagae yukinagae commented Aug 27, 2024

Remove unnecessary nil check as lookup model (ai.LookupModel(provider, name)) returns (*ai.modelActionDef)(nil) when no model is found, which is different from nil in this case.

The below playground shows how it looks like:
https://go.dev/play/p/u-OvxaXc3aZ

Also, this article explains this problem.
https://vitaneri.com/posts/check-for-nil-interface-in-go

Checklist (if applicable):

  • Tested (manually, unit tested, etc.)
  • Changelog updated
  • Docs updated

…ActionDef)(nil)` when no model is found, which is different from `nil`
@yukinagae yukinagae changed the title Remove unnecessary nil check for lookup model fix: remove unnecessary nil check for lookup model Aug 27, 2024
@yukinagae yukinagae changed the title fix: remove unnecessary nil check for lookup model fix(go): remove unnecessary nil check for lookup model Aug 27, 2024
yukinagae added a commit to yukinagae/genkit-golang-openai-sample that referenced this pull request Aug 27, 2024
@MichaelDoyle MichaelDoyle requested a review from apascal07 August 28, 2024 12:48
@yukinagae
Copy link
Contributor Author

I'll close this PR because removing the nil check in this case seems like bad practice. Instead of removing the nil check, I'll modify ai.LookupModel(provider, name) to properly return nil when no model is found.

@yukinagae yukinagae closed this Sep 1, 2024
@yukinagae
Copy link
Contributor Author

I've submitted an issue instead of this PR: #847

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

Successfully merging this pull request may close these issues.

1 participant