[PATCH] devtools: set DTS directory to format check

[PATCH] devtools: set DTS directory to format check

[Thomas Monjalon]

The script was running on the current directory.
If not in the DTS directory, it would re-format every Python files.

A new positional argument is added to specify the directory to check.
In most cases, the (new) default value should be enough.

While updating argument handling,
the usage is printed in case of wrong argument.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 devtools/dts-check-format.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/devtools/dts-check-format.sh b/devtools/dts-check-format.sh
index dc07150775..eb1bdd2a01 100755
--- a/devtools/dts-check-format.sh
+++ b/devtools/dts-check-format.sh
@@ -3,11 +3,10 @@
 # Copyright(c) 2022 University of New Hampshire
 
 usage() {
-	echo "Run formatting and linting programs for DTS. Usage:"
-
+	echo 'Usage: $(basename $0) [options] [directory]'
+	echo 'Options:'
 	# Get source code comments after getopts arguments and print them both
 	grep -E '[a-zA-Z]+\) +#' "$0" | tr -d '#'
-	exit 0
 }
 
 format=true
@@ -17,7 +16,9 @@ lint=true
 while getopts "hfl" arg; do
 	case $arg in
 	h) # Display this message
+		echo 'Run formatting and linting programs for DTS.'
 		usage
+		exit 0
 		;;
 	f) # Don't run formatters
 		format=false
@@ -25,10 +26,15 @@ while getopts "hfl" arg; do
 	l) # Don't run linter
 		lint=false
 		;;
-	*)
+	?)
+		usage
+		exit 1
 	esac
 done
+shift $(($OPTIND - 1))
 
+directory=${1:-$(dirname $0)/../dts}
+cd $directory || exit 1
 
 errors=0
 
-- 
2.36.1

Re: [PATCH] devtools: set DTS directory to format check

[Owen Hilyard]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On Wed, Nov 9, 2022 at 12:09 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> The script was running on the current directory.
> If not in the DTS directory, it would re-format every Python files.
>
> A new positional argument is added to specify the directory to check.
> In most cases, the (new) default value should be enough.
>
> While updating argument handling,
> the usage is printed in case of wrong argument.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  devtools/dts-check-format.sh | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/devtools/dts-check-format.sh b/devtools/dts-check-format.sh
> index dc07150775..eb1bdd2a01 100755
> --- a/devtools/dts-check-format.sh
> +++ b/devtools/dts-check-format.sh
> @@ -3,11 +3,10 @@
>  # Copyright(c) 2022 University of New Hampshire
>
>  usage() {
> -       echo "Run formatting and linting programs for DTS. Usage:"
> -
> +       echo 'Usage: $(basename $0) [options] [directory]'
> +       echo 'Options:'
>         # Get source code comments after getopts arguments and print them
> both
>         grep -E '[a-zA-Z]+\) +#' "$0" | tr -d '#'
> -       exit 0
>  }
>
>  format=true
> @@ -17,7 +16,9 @@ lint=true
>  while getopts "hfl" arg; do
>         case $arg in
>         h) # Display this message
> +               echo 'Run formatting and linting programs for DTS.'
>                 usage
> +               exit 0
>                 ;;
>         f) # Don't run formatters
>                 format=false
> @@ -25,10 +26,15 @@ while getopts "hfl" arg; do
>         l) # Don't run linter
>                 lint=false
>                 ;;
> -       *)
> +       ?)
> +               usage
> +               exit 1
>         esac
>  done
> +shift $(($OPTIND - 1))
>
> +directory=${1:-$(dirname $0)/../dts}
> +cd $directory || exit 1
>
>  errors=0
>
> --
> 2.36.1
>
>
Looks good to me.

Reviewed-by: Owen Hilyard <ohilyard@iol.unh.edu>

[-- Attachment #2: Type: text/html, Size: 2764 bytes --]

RE: [PATCH] devtools: set DTS directory to format check

[Juraj Linkeš]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, November 9, 2022 6:09 PM
> To: dev@dpdk.org
> Cc: Juraj Linkeš <juraj.linkes@pantheon.tech>; Lijuan Tu
> <lijuan.tu@intel.com>; Owen Hilyard <ohilyard@iol.unh.edu>
> Subject: [PATCH] devtools: set DTS directory to format check
> 
> The script was running on the current directory.
> If not in the DTS directory, it would re-format every Python files.
> 
> A new positional argument is added to specify the directory to check.
> In most cases, the (new) default value should be enough.
> 
> While updating argument handling,
> the usage is printed in case of wrong argument.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  devtools/dts-check-format.sh | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/devtools/dts-check-format.sh b/devtools/dts-check-format.sh
> index dc07150775..eb1bdd2a01 100755
> --- a/devtools/dts-check-format.sh
> +++ b/devtools/dts-check-format.sh
> @@ -3,11 +3,10 @@
>  # Copyright(c) 2022 University of New Hampshire
> 
>  usage() {
> -	echo "Run formatting and linting programs for DTS. Usage:"
> -
> +	echo 'Usage: $(basename $0) [options] [directory]'

Double quotes here, otherwise $0 won't be expanded.

> +	echo 'Options:'
>  	# Get source code comments after getopts arguments and print
> them both
>  	grep -E '[a-zA-Z]+\) +#' "$0" | tr -d '#'
> -	exit 0
>  }
> 
>  format=true
> @@ -17,7 +16,9 @@ lint=true
>  while getopts "hfl" arg; do
>  	case $arg in
>  	h) # Display this message
> +		echo 'Run formatting and linting programs for DTS.'
>  		usage
> +		exit 0
>  		;;
>  	f) # Don't run formatters
>  		format=false
> @@ -25,10 +26,15 @@ while getopts "hfl" arg; do
>  	l) # Don't run linter
>  		lint=false
>  		;;
> -	*)
> +	?)
> +		usage
> +		exit 1
>  	esac
>  done
> +shift $(($OPTIND - 1))
> 
> +directory=${1:-$(dirname $0)/../dts}
> +cd $directory || exit 1
> 

I'd like to include the information of where we're doing the fomatting in the console output, e.g.:
echo "Formatting in $(pwd):"

We're silently chaning the directory, so this would be useful when running with no argument and the script doesn't change anything - as a confirmation that it ran over the files we wanted to.

>  errors=0
> 
> --
> 2.36.1
> 

Other than that,
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Tested-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

Re: [PATCH] devtools: set DTS directory to format check

[Thomas Monjalon]

10/11/2022 10:27, Juraj Linkeš:
> From: Thomas Monjalon <thomas@monjalon.net>
> >  usage() {
> > -	echo "Run formatting and linting programs for DTS. Usage:"
> > -
> > +	echo 'Usage: $(basename $0) [options] [directory]'
> 
> Double quotes here, otherwise $0 won't be expanded.

I wonder how I tested it :)

> > +directory=${1:-$(dirname $0)/../dts}
> > +cd $directory || exit 1
> 
> I'd like to include the information of where we're doing the fomatting in the console output, e.g.:
> echo "Formatting in $(pwd):"
> 
> We're silently chaning the directory, so this would be useful when running with no argument and the script doesn't change anything - as a confirmation that it ran over the files we wanted to.

Good comment, I'll improve in v2.

> Other than that,
> Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Tested-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

No it does not work, so you should not add your Tested-by.
And in general, Reviewed-by is enough.
And really, give your reviewed-by only when it's perfect :)

RE: [PATCH] devtools: set DTS directory to format check

[Juraj Linkeš]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, November 10, 2022 11:26 AM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: dev@dpdk.org; Lijuan Tu <lijuan.tu@intel.com>; Owen Hilyard
> <ohilyard@iol.unh.edu>
> Subject: Re: [PATCH] devtools: set DTS directory to format check
> 
> 10/11/2022 10:27, Juraj Linkeš:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > >  usage() {
> > > -	echo "Run formatting and linting programs for DTS. Usage:"
> > > -
> > > +	echo 'Usage: $(basename $0) [options] [directory]'
> >
> > Double quotes here, otherwise $0 won't be expanded.
> 
> I wonder how I tested it :)
> 
> > > +directory=${1:-$(dirname $0)/../dts} cd $directory || exit 1
> >
> > I'd like to include the information of where we're doing the fomatting in
> the console output, e.g.:
> > echo "Formatting in $(pwd):"
> >
> > We're silently chaning the directory, so this would be useful when running
> with no argument and the script doesn't change anything - as a confirmation
> that it ran over the files we wanted to.
> 
> Good comment, I'll improve in v2.

One more thing, if we go with changing the Formatting echo, then we should also change the Linting echo. Or we could do it in some other place just once, I'm not sure which is better.

> 
> > Other than that,
> > Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Tested-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> 
> No it does not work, so you should not add your Tested-by.
> And in general, Reviewed-by is enough.
> And really, give your reviewed-by only when it's perfect :)
> 

Thanks. It actually does work (by which I mean it does format and lint), but it's not perfect, so I guess Tested-by should be fine, but Reviewed-by isn't? :-)

RE: [PATCH] devtools: set DTS directory to format check

[Juraj Linkeš]

> >
> > Good comment, I'll improve in v2.
> 
> One more thing, if we go with changing the Formatting echo, then we should
> also change the Linting echo. Or we could do it in some other place just once,
> I'm not sure which is better.
> 

I have one more request, could you add "echo 'Summary:'" before 'echo "Found $errors errors"' so that it's 100% clear what we're printing at the end (and that it's not part of the 'Linting:' output).

Thanks

Re: [PATCH] devtools: set DTS directory to format check

[Thomas Monjalon]

11/11/2022 11:06, Juraj Linkeš:
> > >
> > > Good comment, I'll improve in v2.
> > 
> > One more thing, if we go with changing the Formatting echo, then we should
> > also change the Linting echo. Or we could do it in some other place just once,
> > I'm not sure which is better.
> 
> I have one more request, could you add "echo 'Summary:'" before 'echo "Found $errors errors"' so that it's 100% clear what we're printing at the end (and that it's not part of the 'Linting:' output).

OK good idea

[PATCH v2] devtools: set DTS directory to format check

[Thomas Monjalon]

The script was running on the current directory.
If not in the DTS directory, it would re-format every Python files.

A new positional argument is added to specify the directory to check.
In most cases, the (new) default value should be enough.

While updating argument handling,
the usage is printed in case of wrong argument.

The directory is also printed in headings,
and a last heading is added for the summary part.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Owen Hilyard <ohilyard@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
v2: add directory in headings and improve headings
---
 devtools/dts-check-format.sh | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/devtools/dts-check-format.sh b/devtools/dts-check-format.sh
index dc07150775..c9b3702642 100755
--- a/devtools/dts-check-format.sh
+++ b/devtools/dts-check-format.sh
@@ -3,11 +3,10 @@
 # Copyright(c) 2022 University of New Hampshire
 
 usage() {
-	echo "Run formatting and linting programs for DTS. Usage:"
-
+	echo "Usage: $(basename $0) [options] [directory]"
+	echo 'Options:'
 	# Get source code comments after getopts arguments and print them both
 	grep -E '[a-zA-Z]+\) +#' "$0" | tr -d '#'
-	exit 0
 }
 
 format=true
@@ -17,7 +16,9 @@ lint=true
 while getopts "hfl" arg; do
 	case $arg in
 	h) # Display this message
+		echo 'Run formatting and linting programs for DTS.'
 		usage
+		exit 0
 		;;
 	f) # Don't run formatters
 		format=false
@@ -25,17 +26,27 @@ while getopts "hfl" arg; do
 	l) # Don't run linter
 		lint=false
 		;;
-	*)
+	?)
+		usage
+		exit 1
 	esac
 done
+shift $(($OPTIND - 1))
 
+directory=$(realpath --relative-base=$(pwd) ${1:-$(dirname $0)/../dts})
+cd $directory || exit 1
+
+heading() {
+	echo $*
+	echo $* | sed 's/./-/g' # underline
+}
 
 errors=0
 
 if $format; then
 	if command -v git > /dev/null; then
 		if git rev-parse --is-inside-work-tree >&-; then
-			echo "Formatting:"
+			heading "Formatting in $directory/"
 			if command -v black > /dev/null; then
 				echo "Formatting code with black:"
 				black .
@@ -72,7 +83,7 @@ if $lint; then
 	if $format; then
 		echo
 	fi
-	echo "Linting:"
+	heading "Linting in $directory/"
 	if command -v pylama > /dev/null; then
 		pylama .
 		errors=$((errors + $?))
@@ -83,5 +94,6 @@ if $lint; then
 fi
 
 echo
+heading "Summary for $directory/"
 echo "Found $errors errors"
 exit $errors
-- 
2.36.1

RE: [PATCH v2] devtools: set DTS directory to format check

[Juraj Linkeš]

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Wednesday, November 23, 2022 12:22 PM
To: dev@dpdk.org
Cc: Juraj Linkeš <juraj.linkes@pantheon.tech>; Owen Hilyard <ohilyard@iol.unh.edu>; Lijuan Tu <lijuan.tu@intel.com>
Subject: [PATCH v2] devtools: set DTS directory to format check

The script was running on the current directory.
If not in the DTS directory, it would re-format every Python files.

A new positional argument is added to specify the directory to check.
In most cases, the (new) default value should be enough.

While updating argument handling,
the usage is printed in case of wrong argument.

The directory is also printed in headings, and a last heading is added for the summary part.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Owen Hilyard <ohilyard@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
v2: add directory in headings and improve headings
---

Nice touch with the headings. The patch looks and works great.

Tested-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

Re: [PATCH v2] devtools: set DTS directory to format check

[Thomas Monjalon]

> The script was running on the current directory.
> If not in the DTS directory, it would re-format every Python files.
> 
> A new positional argument is added to specify the directory to check.
> In most cases, the (new) default value should be enough.
> 
> While updating argument handling,
> the usage is printed in case of wrong argument.
> 
> The directory is also printed in headings, and a last heading is added for the summary part.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Reviewed-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
> v2: add directory in headings and improve headings
> ---
> 
> Nice touch with the headings. The patch looks and works great.

Thanks for the ideas.

> Tested-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

Applied