-
Notifications
You must be signed in to change notification settings - Fork 6
fixed critical bug in configuration and add specs #30
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
base: main
Are you sure you want to change the base?
Conversation
… and functionality
… model attribute for enhanced flexibility
| attr_accessor :openai, | ||
| :anthropic, | ||
| :gemini, | ||
| # default api config accessors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not add comments inside the params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, roger that, that comment will be removed
lib/rubyai/configuration.rb
Outdated
| :temperature, | ||
| :provider | ||
|
|
||
| PROVIDERS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add providers in the top
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move the providers to the top of the file for better organization.
lib/rubyai/provider.rb
Outdated
| module Provider | ||
| PROVIDERS = { | ||
| "openai" => RubyAI::Providers::OpenAI, | ||
| # doesn't tested yet because i don't have an anthropic api key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not add comments like this inside code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, I'll remove all comments like this. Thanks!
lib/rubyai/providers/anthropic.rb
Outdated
| @@ -0,0 +1,55 @@ | |||
| module RubyAI | |||
| module Providers | |||
| # doesn't tested yet because i don't have an anthropic api key | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not add comments like this inside code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I’ll remove the comment from the code. Thanks!
lib/rubyai/providers/gemini.rb
Outdated
|
|
||
| attr_accessor :api, :messages, :temperature, :max_tokens, :model | ||
|
|
||
| # TODO: make an initialization for separate instances for using multiple of them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not add any To-do in comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can open a new ticket/issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood! I’ll remove the TODO comment and create an issue/ticket. Thanks!
| require_relative "rubyai/providers/openai" | ||
| require_relative "rubyai/providers/anthropic" | ||
| require_relative "rubyai/providers/gemini" | ||
| require_relative "rubyai/provider" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, could you please specify what part you mean? I’m not sure what you're referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need require require_relative "rubyai/provider"
if we have required all of them directly?
lib/rubyai/configuration.rb
Outdated
| :temperature, | ||
| :provider | ||
|
|
||
| PROVIDERS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can use the module Provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I agree.
i will update it
| def self.configuration | ||
| @configuration ||= Configuration.new | ||
| end | ||
| def initialize(config = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many variables inside the initialize. Should be <5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I’ll refactor the initialize method.
|
I have a few questions:
|

[ ✅ ] Have you followed the guidelines in our Contributing document?
[ ✅ ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
[ ✅ ] Have you added an explanation of what your changes do and why you'd like us to include them?
Summary:
Notes: