fix: handle exception in dynamic select options when trigger is not registered #281
Conversation
…egistered When using dynamic select options in tools/models, the plugin executor tries to get a trigger event handler even if no trigger is registered. This caused an unhandled exception that broke the dynamic select functionality. Added try-except block to get_trigger_event_handler_safely() to catch and log any exceptions, returning None instead of propagating the error.
Summary of ChangesHello @Mairuis, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug affecting dynamic select options. Previously, if a trigger provider was not registered, the system would encounter an unhandled exception, leading to a breakdown in functionality. The change introduces robust error handling by wrapping the problematic logic in a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an unhandled exception in get_trigger_event_handler_safely by wrapping the logic in a try-except block. My review includes one suggestion to improve the implementation by using the standard logging module for error messages instead of print, which is a best practice for libraries.
| _, event_cls = entry.events[event] | ||
| return event_cls(runtime) | ||
| except Exception as e: | ||
| print(f"Error getting trigger event handler: {e!s}") |
There was a problem hiding this comment.
Using print() for errors in a library is not a best practice. It's better to use the logging module. This allows developers using your library to configure how and where logs are handled (e.g., setting log levels, redirecting to a file or a logging service), which is much more flexible than printing to standard output.
You'll need to add import logging at the top of the file.
As a side note, catching a broad Exception is powerful for a 'safe' function like this, but it can sometimes mask unexpected issues. It's worth being aware of this trade-off.
| print(f"Error getting trigger event handler: {e!s}") | |
| logging.warning(f"Error getting trigger event handler: {e!s}") |
Summary
get_trigger_event_handler_safely()that broke dynamic select optionsProblem
When using
dynamic-selectparameter type in tools or models, the plugin executor attempts to get a trigger event handler even when no trigger is registered. The_get_entry()method raises aValueErrorwhen the provider is not found, and this exception was not caught, breaking the entire dynamic select functionality.Solution
Wrapped the method body in a try-except block to catch any exceptions and return
Nonegracefully. This allows dynamic select to function correctly even when triggers are not involved.Closes #280