chore(ci): enabling A & B rules in Ruff #305
Conversation
There was a problem hiding this comment.
Hey @brnaba-aws I just left some comments explaining what I'm changing.
Thanks
| else: | ||
| if not isinstance(options.client, Anthropic): | ||
| raise ValueError("If streaming is disabled, the provided client must be an Anthropic client") | ||
| elif not isinstance(options.client, Anthropic): |
There was a problem hiding this comment.
Simplifying logic.
| self.client = options.client | ||
| elif self.streaming: |
There was a problem hiding this comment.
Simplifying logic.
| @@ -146,7 +144,7 @@ def _build_input( | |||
| system_prompt: str | |||
| ) -> dict: | |||
| """Build the conversation command with all necessary configurations.""" | |||
| input = { | |||
| json_input = { | |||
There was a problem hiding this comment.
Avoiding variables names that shadow built-in python functions.
| @@ -169,7 +167,7 @@ def _get_max_recursions(self) -> int: | |||
|
|
|||
| async def _handle_streaming( | |||
| self, | |||
| input: dict, | |||
| payload_input: dict, | |||
There was a problem hiding this comment.
Avoiding parameters names that shadow built-in python functions.
| @@ -205,32 +203,30 @@ async def stream_generator(): | |||
| async def _process_with_strategy( | |||
| self, | |||
| streaming: bool, | |||
| input: dict, | |||
| payload_input: dict, | |||
There was a problem hiding this comment.
Avoiding parameters names that shadow built-in python functions.
| @@ -62,7 +62,7 @@ async def process_request( | |||
|
|
|||
| except Exception as error: | |||
| Logger.error(f"Error processing request with agent {agent.name}:{str(error)}") | |||
| raise f"Error processing request with agent {agent.name}:{str(error)}" | |||
| raise f"Error processing request with agent {agent.name}:{str(error)}" from error | |||
There was a problem hiding this comment.
Exceptions should propagate the original error for complete stack trace.
| @@ -253,7 +253,7 @@ def _format_agents_memory(self, agents_history: list[ConversationMessage]) -> st | |||
| return ''.join( | |||
| f"{user_msg.role}:{user_msg.content[0].get('text','')}\n" | |||
| f"{asst_msg.role}:{asst_msg.content[0].get('text','')}\n" | |||
| for user_msg, asst_msg in zip(agents_history[::2], agents_history[1::2]) | |||
| for user_msg, asst_msg in zip(agents_history[::2], agents_history[1::2], strict=True) | |||
There was a problem hiding this comment.
Adding strict parameter to make sure zip function will unpack expected values OR will raise ValueError if not.
| @@ -137,10 +137,13 @@ async def agent_process_request(self, | |||
| user_id: str, | |||
| session_id: str, | |||
| classifier_result: ClassifierResult, | |||
| additional_params: dict[str, str] = {}, | |||
| additional_params: dict[str, str] | None = None, | |||
There was a problem hiding this comment.
Should not add mutable parameters in the method signature. Moving the check to inside the function.
| session_id: str, | ||
| additional_params: dict[str, str] = {}, | ||
| stream_response: bool | None = False) -> AgentResponse: | ||
| async def route_request(self, user_input: str, user_id: str, session_id: str, additional_params: dict[str, str] | None = None, stream_response: bool | None = False) -> AgentResponse: |
There was a problem hiding this comment.
Should not add mutable parameters in the method signature. Moving the check to inside the function.
| @@ -84,7 +84,7 @@ def _extract_properties(self, func: Callable) -> dict[str, dict[str, Any]]: | |||
| param_descriptions[param_name] = description | |||
|
|
|||
| properties = {} | |||
| for param_name, param in sig.parameters.items(): | |||
| for param_name, _param in sig.parameters.items(): | |||
There was a problem hiding this comment.
Variables is not being used, so, add _ to make it clear.
Issue Link (REQUIRED)
Fixes #304
Summary
Changes
This PR enables the following rule categories in our Ruff configuration:
A: Flake8-builtins - Checks for Python built-in names being used as variables/parametersB: Flake8-bugbear - Detects common bugs and design problemsA: Flake8-builtins Rules
These rules prevent shadowing of built-in names like
id,list,dict,input, etc. Shadowing built-ins can lead to:B: Flake8-bugbear Rules
Bugbear rules detect a wide range of common Python issues:
User experience
No changes in use experience
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.