Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.
On 6 April 2016 at 13:53, Russell Bryantwrote: > > > On Wed, Apr 6, 2016 at 3:46 PM, Joe Stringer 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 > > > Acked-by: Russell Bryant > > @@ -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. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.
On 7 April 2016 at 05:29, Aaron Conolewrote: > Joe Stringer writes: > >> 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 >> --- >> v2: Broaden the set of blacklisted files to not check. >> --- >> utilities/checkpatch.py | 17 - >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> 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 > > You need an else condition here; otherwise a patch which orders files > > .c > [blacklisted] > .c > > will skip the second .c length line checks. > > Incremental patch could look something like (completely untested) > below. Thanks for doing this. Thanks for pointing this out, we also need to drop the 'break' otherwise we'll simply stop processing files as soon as one of the blacklist files are hit. I'll send one more round for good measure, in case anything was missed this time around ;-) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.
Joe Stringerwrites: > 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 > --- > v2: Broaden the set of blacklisted files to not check. > --- > utilities/checkpatch.py | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > 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 You need an else condition here; otherwise a patch which orders files .c [blacklisted] .c will skip the second .c length line checks. Incremental patch could look something like (completely untested) below. Thanks for doing this. diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 83d14e8..1579c1f 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -121,6 +121,9 @@ def ovs_checkpatch_parse(text): if filetype in current_file: skip_line_length_check = True break +else: +skip_line_length_check = False lineno = lineno + 1 if len(line) <= 0: > + > 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) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.
On Wed, Apr 6, 2016 at 3:46 PM, Joe Stringerwrote: > 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 > Acked-by: Russell Bryant @@ -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 -- Russell Bryant ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv2] checkpatch: Don't enforce char limit on tests.
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--- v2: Broaden the set of blacklisted files to not check. --- utilities/checkpatch.py | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) 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) -- 2.1.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev