-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Use smarter type detection for AssignableTypes #2938
base: master
Are you sure you want to change the base?
Conversation
@@ -18,7 +18,10 @@ module ClassMethods | |||
def load_types_in(module_name, my_name = module_name.singularize) | |||
const_set(:MODULE_NAME, module_name) | |||
const_set(:BASE_CLASS_NAME, my_name) | |||
const_set(:TYPES, Dir[Rails.root.join("app", "models", module_name.underscore, "*.rb")].map { |path| module_name + "::" + File.basename(path, ".rb").camelize }) | |||
types = module_name.constantize.constants |
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'm afraid this may not work in development and test environments where files are lazy-loaded. Can you confirm this works even in development mode? (e.g. bundle exec rails r 'puts Agent::TYPES'
)
@gooroodev please take a look |
Thanks, @admsev! Let's do this! 1. Summary of ChangesThe pull request updates the method
2. Issues, Bugs, or TyposIssue 1: Potential Inaccuracy in Type DetectionThe new type detection method filters constants based on whether their names include the singular name of the module. This could potentially miss some valid types or include irrelevant ones if the naming convention is not strictly followed. Improvement Proposal: types = module_name.constantize.constants
.select { |t| t.to_s.start_with?(my_name) || t.to_s.end_with?(my_name) }
.map { |t| "#{module_name}::#{t}" } Issue 2: Missing Newline at End of FileThe file is missing a newline at the end, which is a common convention in many coding standards. Improvement Proposal: 3. General Review of Code Quality and StyleCode Quality
Code Style
Potential Improvements
ConclusionThe pull request introduces a more dynamic approach to type detection, which is a positive change. Addressing the potential inaccuracy in detection and adding a newline at the end of the file will further improve the quality of the code. Yours, Gooroo.dev. Got thoughts? Don't hesitate to reply or add a reaction. |
Hi there,
This PR stems from an issue I've asked about in Gitter but didn't end up with a response, so I figured I'd submit the fix myself.
When developing agents locally and importing them via the
ADDITIONAL_GEMS=some_agent(path:../some_agent)
pathway, I noticed that any agents within the gems wouldn't end up in the agents list of the agent creation page. Doing some digging, I found that the reason was that the dropdown was being populated, via various layers of the stack, by this line inassignable_types.rb
:Searching through the
app/models/agents
directory naturally omits agents provided by gems, as they're stored not only outside of that directory, but even outside of the Huginn project root. This PR fixes this by populating theTYPES
constant with the actual list of constants contained by the namespace at runtime.The one thing I'm a little stumped on is how this list ends up getting populated properly (ie including agens in gems) when in production, as this is an issue I've only been able to replicate on my development instance. Perhaps there's something I'm missing here, but otherwise this seems like a pretty straightforward fix.