Re: [PATCH v6 2/2] dts: add flow create/delete to testpmd shell

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

Hi Dean, it looks mostly good, just some nits.

On 21/01/2025 20:41, Dean Marx wrote:
> +    def flow_create(self, flow_rule: FlowRule, port_id: int, verify: bool = True) -> int:
> +        """Creates a flow rule in the testpmd session.
> +
> +        Args:
> +            flow_rule: :class:`FlowRule` object used for creating testpmd flow rule.
> +            port_id: Integer representing the port to use.
> +            verify: If :data:`True`, the output of the command is scanned
> +            to ensure the flow rule was created successfully.

This line should be indented further.

> +
> +        Raises:
> +            InteractiveCommandExecutionError: If flow rule is invalid.
> +
> +        Returns:
> +            Id of created flow rule as an integer.
There is no reason to specify the type when it's already annotated as 
part of the function signature.
> +        """
> +        flow_output = self.send_command(f"flow create {port_id} {flow_rule}")
> +        if verify:
> +            if "created" not in flow_output:
> +                self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> +                raise InteractiveCommandExecutionError(
> +                    f"Failed to create flow rule:\n{flow_output}"
> +                )

With the check below here, we are already verifying the command 
execution... as you are effectively testing the same output. Therefore 
this verification above is redundant. I'd remove it, together with the 
verify argument. Finally, I'd specify in the description of the 
docstring that this function by returning the number of the created flow 
it's implicitly verifying its execution.

> +        match = re.search(r"#(\d+)", flow_output)
> +        if match is not None:
> +            match_str = match.group(1)
> +            flow_id = int(match_str)
> +            return flow_id
> +        else:
> +            self._logger.debug(f"Failed to create flow rule:\n{flow_output}")
> +            raise InteractiveCommandExecutionError(f"Failed to create flow rule:\n{flow_output}")
> +
> +    def flow_delete(self, flow_id: int, port_id: int, verify: bool = True) -> None:
> +        """Deletes the specified flow rule from the testpmd session.
> +
> +        Args:
> +            flow_id: :class:`FlowRule` id used for deleting testpmd flow rule.

I guess it's not really a FlowRule id. Just Flow id. So:

   Id of the flow to remove.

> +            port_id: Integer representing the port to use.
> +            verify: If :data:`True`, the output of the command is scanned
> +            to ensure the flow rule was deleted successfully.
indent