diff mbox series

[v3,1/2] Enable codespell by default. Can be disabled from config file.

Message ID 20190301170806.3547-2-msantana@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Minor changes to checkpatches |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Michael Santana March 1, 2019, 5:08 p.m. UTC 
  Enable codespell by default.
codespell is a feature by checkpatch.pl that
checks for common spelling mistakes in patches.

This feature is disabled by default. To enable it one must add
the '--codespell' flag to the $options variable in
checkpatches.sh. With this change codespell is enabled by default.
The user can decide to turn off codespell from a one of the config
files read by checkpatches.sh.

Signed-off-by: Michael Santana <msantana@redhat.com>
Reviewed-by: Rami Rosen <ramirose@gmail.com>
---
v2->v3:
  Also enable codespell by setting a path to a dictionary file.

 devtools/checkpatches.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Thomas Monjalon March 1, 2019, 5:43 p.m. UTC | #1 
01/03/2019 18:08, Michael Santana:
> +# Enable codespell by default. This can be overwritten from a config file.
> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
> +# to a dictionary.txt file if your dictionary.txt is not in the default location.

Better to avoid "you" form in such comment.

> +DPDK_CHECKPATCH_CODESPELL=enable
>  # Load config options:
>  # - DPDK_CHECKPATCH_PATH
>  # - DPDK_CHECKPATCH_LINE_LENGTH
> +# - DPDK_CHECKPATCH_CODESPELL
>  . $(dirname $(readlink -e $0))/load-devel-config
>  
>  VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
>  
>  # override default Linux options
>  options="--no-tree"
> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then

This is a bashism.
Standard sh uses a simple =

No need for a v4, I can fix it.
Thomas Monjalon March 1, 2019, 5:51 p.m. UTC | #2 
01/03/2019 18:43, Thomas Monjalon:
> 01/03/2019 18:08, Michael Santana:
> > +# Enable codespell by default. This can be overwritten from a config file.
> > +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
> > +# to a dictionary.txt file if your dictionary.txt is not in the default location.

This line length won't pass chekpatch ;)

> Better to avoid "you" form in such comment.
> 
> > +DPDK_CHECKPATCH_CODESPELL=enable

It will override the value if passed with an environment variable.
You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.

> >  # Load config options:
> >  # - DPDK_CHECKPATCH_PATH
> >  # - DPDK_CHECKPATCH_LINE_LENGTH
> > +# - DPDK_CHECKPATCH_CODESPELL
> >  . $(dirname $(readlink -e $0))/load-devel-config
> >  
> >  VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
> > @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
> >  
> >  # override default Linux options
> >  options="--no-tree"
> > +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
> 
> This is a bashism.
> Standard sh uses a simple =
> 
> No need for a v4, I can fix it.

Because of the required change for the env var case,
please do a v4.
Michael Santana March 1, 2019, 8:24 p.m. UTC | #3 
On 3/1/19 12:51 PM, Thomas Monjalon wrote:
> 01/03/2019 18:43, Thomas Monjalon:
>> 01/03/2019 18:08, Michael Santana:
>>> +# Enable codespell by default. This can be overwritten from a config file.
>>> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
>>> +# to a dictionary.txt file if your dictionary.txt is not in the default location.
> This line length won't pass chekpatch ;)
>
>> Better to avoid "you" form in such comment.
>>
>>> +DPDK_CHECKPATCH_CODESPELL=enable
> It will override the value if passed with an environment variable.
> You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.
If I understood you correctly, you want to be able to set these 
parameters via environment variables (and take precedence over any 
variables set in any config file)
The problem is right now that any environment variable is overwritten by 
the variable being set in one of the config files
The only way I can think of doing this would be by saving the DPDK 
variables (passed via environment) to a file or ironically temporary 
variables (which themselves can also be overwritten, so that doesn't 
really solve the problem) before being overwritten, and then restoring 
said variables after the call to source.
This would add extra clutter in checkpatches, but it can be avoided by 
doing it in load-devel-config instead.

So the bottom line is, environment variables take overall precedence, 
then config files, and then default

Does this sound sane enough?
If anyone knows a better way to do this please share.

I am including in DPDK_CHECKPATCH_PATH, because might as well at this 
point.
>
>>>   # Load config options:
>>>   # - DPDK_CHECKPATCH_PATH
>>>   # - DPDK_CHECKPATCH_LINE_LENGTH
>>> +# - DPDK_CHECKPATCH_CODESPELL
>>>   . $(dirname $(readlink -e $0))/load-devel-config
>>>   
>>>   VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
>>> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
>>>   
>>>   # override default Linux options
>>>   options="--no-tree"
>>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
>> This is a bashism.
>> Standard sh uses a simple =
>>
>> No need for a v4, I can fix it.
> Because of the required change for the env var case,
> please do a v4.
>
>
Thomas Monjalon March 1, 2019, 9:08 p.m. UTC | #4 
01/03/2019 21:24, Michael Santana Francisco:
> On 3/1/19 12:51 PM, Thomas Monjalon wrote:
> > 01/03/2019 18:43, Thomas Monjalon:
> >> 01/03/2019 18:08, Michael Santana:
> >>> +# Enable codespell by default. This can be overwritten from a config file.
> >>> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
> >>> +# to a dictionary.txt file if your dictionary.txt is not in the default location.
> > This line length won't pass chekpatch ;)
> >
> >> Better to avoid "you" form in such comment.
> >>
> >>> +DPDK_CHECKPATCH_CODESPELL=enable
> > It will override the value if passed with an environment variable.
> > You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.
> If I understood you correctly, you want to be able to set these 
> parameters via environment variables (and take precedence over any 
> variables set in any config file)

No :)
The scenario is to have no config file and use environment variables only.

> The problem is right now that any environment variable is overwritten by 
> the variable being set in one of the config files
> The only way I can think of doing this would be by saving the DPDK 
> variables (passed via environment) to a file or ironically temporary 
> variables (which themselves can also be overwritten, so that doesn't 
> really solve the problem) before being overwritten, and then restoring 
> said variables after the call to source.
> This would add extra clutter in checkpatches, but it can be avoided by 
> doing it in load-devel-config instead.
> 
> So the bottom line is, environment variables take overall precedence, 
> then config files, and then default
> 
> Does this sound sane enough?
> If anyone knows a better way to do this please share.

Look how DPDK_CHECKPATCH_LINE_LENGTH is handled.
The default value is used if DPDK_CHECKPATCH_LINE_LENGTH is not set,
neither by environment nor config file.

I think you can just do this after loading config file:

DPDK_CHECKPATCH_CODESPELL=${DPDK_CHECKPATCH_CODESPELL:-enable}

or check for empty value in the test:

[ -z "$DPDK_CHECKPATCH_CODESPELL" -o "$DPDK_CHECKPATCH_CODESPELL" = enable ]


> I am including in DPDK_CHECKPATCH_PATH, because might as well at this 
> point.

Empty DPDK_CHECKPATCH_PATH is already handled.

> >
> >>>   # Load config options:
> >>>   # - DPDK_CHECKPATCH_PATH
> >>>   # - DPDK_CHECKPATCH_LINE_LENGTH
> >>> +# - DPDK_CHECKPATCH_CODESPELL
> >>>   . $(dirname $(readlink -e $0))/load-devel-config
> >>>   
> >>>   VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
> >>> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
> >>>   
> >>>   # override default Linux options
> >>>   options="--no-tree"
> >>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
> >> This is a bashism.
> >> Standard sh uses a simple =
> >>
> >> No need for a v4, I can fix it.
> > Because of the required change for the env var case,
> > please do a v4.
Michael Santana March 4, 2019, 4:59 p.m. UTC | #5 
On 3/1/19 4:08 PM, Thomas Monjalon wrote:
> 01/03/2019 21:24, Michael Santana Francisco:
>> On 3/1/19 12:51 PM, Thomas Monjalon wrote:
>>> 01/03/2019 18:43, Thomas Monjalon:
>>>> 01/03/2019 18:08, Michael Santana:
>>>>> +# Enable codespell by default. This can be overwritten from a config file.
>>>>> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
>>>>> +# to a dictionary.txt file if your dictionary.txt is not in the default location.
>>> This line length won't pass chekpatch ;)
>>>
>>>> Better to avoid "you" form in such comment.
>>>>
>>>>> +DPDK_CHECKPATCH_CODESPELL=enable
>>> It will override the value if passed with an environment variable.
>>> You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.
>> If I understood you correctly, you want to be able to set these
>> parameters via environment variables (and take precedence over any
>> variables set in any config file)
> No :)
> The scenario is to have no config file and use environment variables only.
>
>> The problem is right now that any environment variable is overwritten by
>> the variable being set in one of the config files
>> The only way I can think of doing this would be by saving the DPDK
>> variables (passed via environment) to a file or ironically temporary
>> variables (which themselves can also be overwritten, so that doesn't
>> really solve the problem) before being overwritten, and then restoring
>> said variables after the call to source.
>> This would add extra clutter in checkpatches, but it can be avoided by
>> doing it in load-devel-config instead.
>>
>> So the bottom line is, environment variables take overall precedence,
>> then config files, and then default
>>
>> Does this sound sane enough?
>> If anyone knows a better way to do this please share.
> Look how DPDK_CHECKPATCH_LINE_LENGTH is handled.
> The default value is used if DPDK_CHECKPATCH_LINE_LENGTH is not set,
> neither by environment nor config file.
>
> I think you can just do this after loading config file:
>
> DPDK_CHECKPATCH_CODESPELL=${DPDK_CHECKPATCH_CODESPELL:-enable}
Oh! So it's much simpler than I thought. Yeah. I will remove what I have 
now and put in your line to go along with the length= line. Will also 
change the comment as I didn't remove the 'you' form in the last commit
>
> or check for empty value in the test:
>
> [ -z "$DPDK_CHECKPATCH_CODESPELL" -o "$DPDK_CHECKPATCH_CODESPELL" = enable ]
>
>
>> I am including in DPDK_CHECKPATCH_PATH, because might as well at this
>> point.
> Empty DPDK_CHECKPATCH_PATH is already handled.
>
>>>>>    # Load config options:
>>>>>    # - DPDK_CHECKPATCH_PATH
>>>>>    # - DPDK_CHECKPATCH_LINE_LENGTH
>>>>> +# - DPDK_CHECKPATCH_CODESPELL
>>>>>    . $(dirname $(readlink -e $0))/load-devel-config
>>>>>    
>>>>>    VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
>>>>> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
>>>>>    
>>>>>    # override default Linux options
>>>>>    options="--no-tree"
>>>>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
>>>> This is a bashism.
>>>> Standard sh uses a simple =
>>>>
>>>> No need for a v4, I can fix it.
>>> Because of the required change for the env var case,
>>> please do a v4.
>
>
diff mbox series

Patch

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 3b03b7ef2..d3c0b309a 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -2,9 +2,14 @@ 
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright 2015 6WIND S.A.
 
+# Enable codespell by default. This can be overwritten from a config file.
+# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
+# to a dictionary.txt file if your dictionary.txt is not in the default location.
+DPDK_CHECKPATCH_CODESPELL=enable
 # Load config options:
 # - DPDK_CHECKPATCH_PATH
 # - DPDK_CHECKPATCH_LINE_LENGTH
+# - DPDK_CHECKPATCH_CODESPELL
 . $(dirname $(readlink -e $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
@@ -13,6 +18,12 @@  length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
 options="--no-tree"
+if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
+    options="$options --codespell"
+elif [ -f "$DPDK_CHECKPATCH_CODESPELL" ]; then
+    options="$options --codespell"
+    options="$options --codespellfile $DPDK_CHECKPATCH_CODESPELL"
+fi
 options="$options --max-line-length=$length"
 options="$options --show-types"
 options="$options --ignore=LINUX_VERSION_CODE,\