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

[Dean Marx <dmarx@iol.unh.edu>]

On Wed, Aug 27, 2025 at 11:46 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> 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.

Heard

>
> > +# 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"

Thanks for catching this, I hadn't realized

>
> > +
> > +
> > +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"}

I'll add this to the new version

>
> > +            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.

Okay, I'll add a docstringcheck variable in the check-format script to
give it its own if block