Re: [PATCH v2] dts: add test case docstring checks to format script

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

Hi Dean,

thank you for your patch! Looks mostly good, a couple of comments
though.

On Tue, Aug 26, 2025 at 04:07:20PM +0000, Dean Marx wrote:
> diff --git a/devtools/dts-check-docstrings.py b/devtools/dts-check-docstrings.py
> new file mode 100755
> index 0000000000..0baee6e383
> --- /dev/null
> +++ b/devtools/dts-check-docstrings.py
> @@ -0,0 +1,52 @@

Consider adding a shebang here, like it's already done with the other
scripts.

> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2025 University of New Hampshire
> +
> +import sys
> +from ast import FunctionDef, Name, walk, get_docstring, parse
> +from pathlib import Path
> +
> +BASE_DIR = Path(__file__).resolve().parent  # dpdk/

The hint is actually wrong, this is dpdk/devtools. I think you meant to
make this:

    BASE_DIR = Path(__file__).resolve().parent.parent

> +TESTS_DIR = BASE_DIR.parent / "dts" / "tests"  # dts/tests/

and this would just be:

    TESTS_DIR = BASE_DIR / "dts" / "tests"

> +
> +
> +def has_test_decorator(node: FunctionDef) -> bool:
> +    """Return True if function has @func_test or @perf_test decorator."""
> +    for decorator in node.decorator_list:
> +        if isinstance(decorator, Name) and decorator.id in {"func_test", "perf_test"}:

I wouldn't hard code the decorator names like this. Put this in a
constant at the top instead:

    DECORATOR_NAMES = {"func_test", "perf_test"}

> +            return True
> +    return False
<snip>
> diff --git a/devtools/dts-check-format.sh b/devtools/dts-check-format.sh
> index 907eed1293..da6e6f34ee 100755
> --- a/devtools/dts-check-format.sh
> +++ b/devtools/dts-check-format.sh
> @@ -86,7 +86,14 @@ if $lint; then
>  		ruff check --fix
>  		errors=$((errors + $?))
>  
> +		docstring_script_path=$(dirname "$0")
> +		docstring_script_path=$(cd "$docstring_script_path" && pwd)
> +		docstring_script="$docstring_script_path/dts-check-docstrings.py"
> +		python "$docstring_script"
> +		errors=$((errors + $?))
> +

I wouldn't insert this with the ruff if scope. This can still stay under
the lint if block, but give it its own.

>  		git update-index --refresh
> +

Best regards,
Luca