-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add docstrings to utility methods in agent.py #3035
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
Disclaimer: This review was made by a crew of AI Agents. Code Review for crewAIInc/crewAI PR #3035 — Adding Missing Docstrings to Utility Methods in
|
Disclaimer: This review was made by a crew of AI Agents. Code Review Comments for PR #3035OverviewThis pull request makes significant improvements to the documentation of utility methods in
Positive Aspects
Areas for ImprovementWhile the pull request makes commendable improvements, there are some areas that could be enhanced further: 1. get_delegation_tools()Improvement Suggestions:
Example: def get_delegation_tools(self, agents: List[BaseAgent]) -> List[BaseTool]:
"""
Retrieve delegation tools configured with the provided agents.
Args:
agents (List[BaseAgent]): A list of agents to be used for delegation.
Returns:
List[BaseTool]: Tools associated with delegation through the given agents.
Raises:
ValueError: If agents list is empty or contains invalid agent types.
""" 2. get_multimodal_tools()Improvement Suggestions:
Example: def get_multimodal_tools(self) -> Sequence[BaseTool]:
"""
Retrieve tools used for handling multimodal inputs like images.
Returns:
Sequence[BaseTool]: Currently includes AddImageTool for image handling capabilities.
Note:
This method currently only supports image handling through AddImageTool.
""" 3. get_code_execution_tools()Improvement Suggestions:
Example: def get_code_execution_tools(self) -> Optional[List[BaseTool]]:
"""
Retrieve tools for code execution.
Returns:
List[BaseTool] | None: List of code execution tools if available,
otherwise None if crewai_tools is not installed.
Raises:
ImportError: When crewai_tools package is not installed.
Note:
Requires crewai_tools package to be installed for functionality.
""" 4. get_output_converter()Improvement Suggestions:
Example: def get_output_converter(
self,
llm: BaseLLM,
text: str,
model: str,
instructions: str
) -> Converter:
"""
Create a Converter object for processing model outputs.
Args:
llm (BaseLLM): The language model instance to be used for conversion.
Returns:
Converter: Configured Converter object.
""" Historical Context from Related PRsPrevious discussions in pull requests regarding documentation have highlighted the necessity for:
In particular, PRs that have focused on documenting return types and error handling consistently underscore the value this brings to code maintainability and comprehension. General Recommendations
SummaryThe improvements in this PR are a solid step toward establishing clearer documentation standards within the codebase. By addressing the areas for improvement, particularly around exception handling and usage examples, we can further elevate the quality and maintainability of the documentation. I recommend implementing these suggestions in follow-up PRs to uphold and enhance the documentation quality moving forward. |
Thank you so much for the detailed and constructive reviews! I’m glad the improvements were helpful. I’ll go ahead and make a follow-up PR implementing the raised suggestions — including refining the return types, adding exception details, and enhancing docstring clarity for conditional dependencies. That way, this PR can stay clean and focused. Let me know if you’d prefer the changes here instead — I’m happy to update either way. |
Hi again 👋 Just following up to see if it's okay to proceed with a follow-up PR for the suggested docstring refinements (like exception handling, improved types, etc.). Happy to make those changes in a separate PR — just wanted to confirm before opening it. Thanks again for your time and feedback! 🙏 |
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.
@dhyeyinf great changes!
However can you add the type hints also?
Thank you so much for the feedback! 🙏 Please let me know if there’s anything else you'd like me to adjust — happy to make further changes if needed. |
Thanks again for the review and guidance! 🙌 Let me know if there's anything else you'd like me to improve — happy to iterate further. |
Hi @lucasgomide 👋 Just following up on this — I’ve addressed the Let me know if there's anything else you'd like me to update. Otherwise, if everything looks good, this should be ready for merge. Thanks again! 😊 |
@dhyeyinf I'm sorry about delay to reply but now you have to resolve some conflicts. Please tag me when you did it |
What this PR does
This pull request adds missing docstrings to several utility methods in
src/crewai/agent.py
to improve code readability and developer understanding.Updated Methods
get_delegation_tools(self, agents)
get_multimodal_tools(self)
get_code_execution_tools(self)
get_output_converter(self, llm, text, model, instructions)
These docstrings follow PEP 257 standards and are aimed at helping contributors and users better understand the codebase.
Why this is important
Improved documentation supports open-source maintainability and makes it easier for new contributors to onboard. These utility functions previously lacked docstrings, which could lead to confusion about their purpose or return types.
Additional Notes
No functionality was changed. The file passes
ruff
andblack
formatting checks.Please feel free to suggest any improvements — I’d be happy to make changes if needed. 🙂