[ovs-dev] checkpatch: Be more specific about line length, misspelling warnings.

Until now checkpatch warnings have not said how long a too-long line is
or what word might be misspelled.  This commit makes the messages more
explicit.

To do this the 'print' functions needed to know the line that was in error.
One way to do that was to also pass the line in question to the 'print'
function.  I decided instead to just allow the 'print' function to be
missing and to instead issue these warnings from the 'check' function.  I
don't know whether this design raises any red flags with anyone.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 utilities/checkpatch.py | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Ben Pfaff <blp@ovn.org> writes:

> Until now checkpatch warnings have not said how long a too-long line is
> or what word might be misspelled.  This commit makes the messages more
> explicit.
>
> To do this the 'print' functions needed to know the line that was in error.
> One way to do that was to also pass the line in question to the 'print'
> function.  I decided instead to just allow the 'print' function to be
> missing and to instead issue these warnings from the 'check' function.  I
> don't know whether this design raises any red flags with anyone.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---

Thanks, Ben!

I looked into one alternative which is to use the 'inspect' module and
populate arguments to the print function if it took them.  Then I
realized that way lies the path to madness, as it will require a lot of
care and feeding for little benefit that I see right now.  So with that,
I'm fine with warning prints happening in the check functions.  If it
proves to be too burdensome, we can always investigate another way of
structuring it.  For now, I especially appreciate the additional
specificity in the misspelling check.

I lost my group of broken patches that I used to test, so until I rig
up a few I hope that my limited run is okay.  I have attached an
incremental to fold in that fixes an error in the print call.

Acked-by: Aaron Conole <aconole@redhat.com>

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 9b369faf9..3ff54bb96 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -588,8 +588,8 @@ def run_checks(current_file, line, lineno):
         if 'prereq' in check and not check['prereq'](line):
             continue
         if check['check'](line):
-            if 'print' in line:
-                check['print'](line)
+            if 'print' in check:
+                check['print']()
             print_line = True
 
     if print_line:


>  utilities/checkpatch.py | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 1438e5aa4401..9b369faf9287 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -264,6 +264,8 @@ def pointer_whitespace_check(line):
>  def line_length_check(line):
>      """Return TRUE if the line length is too long"""
>      if len(line) > 79:
> +        print_warning("Line is %d characters long (recommended limit is 79)"
> +                      % len(line))
>          return True
>      return False
>  
> @@ -373,6 +375,8 @@ def check_comment_spelling(line):
>                  skip = True
>  
>              if not skip:
> +                print_warning("Check for spelling mistakes (e.g. \"%s\")"
> +                              % strword)
>                  return True
>  
>      return False
> @@ -460,8 +464,7 @@ checks = [
>      {'regex': None,
>       'match_name':
>       lambda x: not line_length_blacklist.match(x),
> -     'check': lambda x: line_length_check(x),
> -     'print': lambda: print_warning("Line length is >79-characters long")},
> +     'check': lambda x: line_length_check(x)},
>  
>      {'regex': None,
>       'match_name':
> @@ -502,8 +505,7 @@ checks = [
>  
>      {'regex': '(\.c|\.h)(\.in)?$', 'match_name': None,
>       'prereq': lambda x: has_comment(x),
> -     'check': lambda x: check_comment_spelling(x),
> -     'print': lambda: print_warning("Check for spelling mistakes")},
> +     'check': lambda x: check_comment_spelling(x)},
>  ]
>  
>  
> @@ -586,7 +588,8 @@ def run_checks(current_file, line, lineno):
>          if 'prereq' in check and not check['prereq'](line):
>              continue
>          if check['check'](line):
> -            check['print']()
> +            if 'print' in line:
> +                check['print'](line)
>              print_line = True
>  
>      if print_line:

On Tue, May 15, 2018 at 05:05:38PM -0400, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > Until now checkpatch warnings have not said how long a too-long line is
> > or what word might be misspelled.  This commit makes the messages more
> > explicit.
> >
> > To do this the 'print' functions needed to know the line that was in error.
> > One way to do that was to also pass the line in question to the 'print'
> > function.  I decided instead to just allow the 'print' function to be
> > missing and to instead issue these warnings from the 'check' function.  I
> > don't know whether this design raises any red flags with anyone.
> >
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> 
> Thanks, Ben!
> 
> I looked into one alternative which is to use the 'inspect' module and
> populate arguments to the print function if it took them.  Then I
> realized that way lies the path to madness, as it will require a lot of
> care and feeding for little benefit that I see right now.  So with that,
> I'm fine with warning prints happening in the check functions.  If it
> proves to be too burdensome, we can always investigate another way of
> structuring it.  For now, I especially appreciate the additional
> specificity in the misspelling check.
> 
> I lost my group of broken patches that I used to test, so until I rig
> up a few I hope that my limited run is okay.  I have attached an
> incremental to fold in that fixes an error in the print call.
> 
> Acked-by: Aaron Conole <aconole@redhat.com>

Thanks for the review and the fix (I'm an idiot).

I applied this to master.