diff mbox

[ovs-dev,PATCHv2] checkpatch: Don't enforce char limit on tests.

Message ID 1459972000-2654-1-git-send-email-joe@ovn.org
State Changes Requested
Headers show

Commit Message

Joe Stringer April 6, 2016, 7:46 p.m. UTC
Although tests ideally also stick to shorter line lengths, it is very
common for fixed text blocks like flows or large packets to be specified
within tests. Checkpatch shouldn't complain about cases like these.

Signed-off-by: Joe Stringer <joe@ovn.org>
---
v2: Broaden the set of blacklisted files to not check.
---
 utilities/checkpatch.py | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Russell Bryant April 6, 2016, 8:53 p.m. UTC | #1
On Wed, Apr 6, 2016 at 3:46 PM, Joe Stringer <joe@ovn.org> wrote:

> Although tests ideally also stick to shorter line lengths, it is very
> common for fixed text blocks like flows or large packets to be specified
> within tests. Checkpatch shouldn't complain about cases like these.
>
> Signed-off-by: Joe Stringer <joe@ovn.org>
>

Acked-by: Russell Bryant <russell@ovn.org>

 @@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):

>      co_authors = []
>      parse = 0
>      current_file = ''
> +    previous_file = ''
>      scissors = re.compile(r'^[\w]*---[\w]*')
>      hunks = re.compile('^(---|\+\+\+) (\S+)')
>      is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>                                re.I | re.M | re.S)
>      is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>                                re.I | re.M | re.S)
> +    skip_line_length_check = False
>
>      for line in text.split('\n'):
> +        if current_file is not previous_file:
> +            previous_file = current_file
> +            for filetype in line_length_blacklist:
> +                if filetype in current_file:
> +                    skip_line_length_check = True
> +                    break
> +
>

Since you made a comment about Python style, the above could also be:

    if current_file != previous_file:
        previous_file = current_file
        if any([ft in current_file for ft in line_length_blacklist]):
            skip_line_length_check = True
            break
Joe Stringer April 7, 2016, 5:47 p.m. UTC | #2
On 6 April 2016 at 13:53, Russell Bryant <russell@ovn.org> wrote:
>
>
> On Wed, Apr 6, 2016 at 3:46 PM, Joe Stringer <joe@ovn.org> wrote:
>>
>> Although tests ideally also stick to shorter line lengths, it is very
>> common for fixed text blocks like flows or large packets to be specified
>> within tests. Checkpatch shouldn't complain about cases like these.
>>
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>
>
> Acked-by: Russell Bryant <russell@ovn.org>
>
>  @@ -99,14 +105,23 @@ def ovs_checkpatch_parse(text):
>>
>>      co_authors = []
>>      parse = 0
>>      current_file = ''
>> +    previous_file = ''
>>      scissors = re.compile(r'^[\w]*---[\w]*')
>>      hunks = re.compile('^(---|\+\+\+) (\S+)')
>>      is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
>>                                re.I | re.M | re.S)
>>      is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
>>                                re.I | re.M | re.S)
>> +    skip_line_length_check = False
>>
>>      for line in text.split('\n'):
>> +        if current_file is not previous_file:
>> +            previous_file = current_file
>> +            for filetype in line_length_blacklist:
>> +                if filetype in current_file:
>> +                    skip_line_length_check = True
>> +                    break
>> +
>
>
> Since you made a comment about Python style, the above could also be:
>
>     if current_file != previous_file:
>         previous_file = current_file
>         if any([ft in current_file for ft in line_length_blacklist]):
>             skip_line_length_check = True
>             break

I'll fold this in (minus the break line) and send v3.
diff mbox

Patch

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index d9011f370816..83d14e89269e 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -56,6 +56,12 @@  skip_trailing_whitespace_check = False
 skip_block_whitespace_check = False
 skip_signoff_check = False
 
+# Don't enforce character limit on files that include these characters in their
+# name, as they may have legitimate reasons to have longer lines.
+#
+# Python isn't checked as flake8 performs these checks during build.
+line_length_blacklist = ['.am', '.at', 'etc', '.in', '.mk', '.patch', '.py']
+
 
 def is_added_line(line):
     """Returns TRUE if the line in question is an added line.
@@ -99,14 +105,23 @@  def ovs_checkpatch_parse(text):
     co_authors = []
     parse = 0
     current_file = ''
+    previous_file = ''
     scissors = re.compile(r'^[\w]*---[\w]*')
     hunks = re.compile('^(---|\+\+\+) (\S+)')
     is_signature = re.compile(r'((\s*Signed-off-by: )(.*))$',
                               re.I | re.M | re.S)
     is_co_author = re.compile(r'(\s*(Co-authored-by: )(.*))$',
                               re.I | re.M | re.S)
+    skip_line_length_check = False
 
     for line in text.split('\n'):
+        if current_file is not previous_file:
+            previous_file = current_file
+            for filetype in line_length_blacklist:
+                if filetype in current_file:
+                    skip_line_length_check = True
+                    break
+
         lineno = lineno + 1
         if len(line) <= 0:
             continue
@@ -154,7 +169,7 @@  def ovs_checkpatch_parse(text):
             if trailing_whitespace_or_crlf(line[1:]):
                 print_line = True
                 print_warning("Line has trailing whitespace", lineno)
-            if len(line[1:]) > 79:
+            if len(line[1:]) > 79 and not skip_line_length_check:
                 print_line = True
                 print_warning("Line is greater than 79-characters long",
                               lineno)