| Message ID | 1459880222-11303-1-git-send-email-joe@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Apr 5, 2016 at 2:17 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> Some thoughts below ... --- > utilities/checkpatch.py | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index d9011f370816..791c7d902fa5 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -154,7 +154,10 @@ 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: > + > + # Don't enforce character limit on test files, since they > commonly > + # include long pieces of text like flows or hex dumps of > packets > + if len(line[1:]) > 79 and '.at' not in current_file: > print_line = True > print_warning("Line is greater than 79-characters long", > lineno) I believe there are other examples that would hit this same problem, such as *.am at least. flake8 is already checking the equivalent for Python files. An alternative would be to just whitelist what we want to check. (.c, .h, .md?)
Russell Bryant <russell@ovn.org> writes: > On Tue, Apr 5, 2016 at 2:17 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> > > Some thoughts below ... > > --- >> utilities/checkpatch.py | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py >> index d9011f370816..791c7d902fa5 100755 >> --- a/utilities/checkpatch.py >> +++ b/utilities/checkpatch.py >> @@ -154,7 +154,10 @@ 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: >> + >> + # Don't enforce character limit on test files, since they >> commonly >> + # include long pieces of text like flows or hex dumps of >> packets >> + if len(line[1:]) > 79 and '.at' not in current_file: >> print_line = True >> print_warning("Line is greater than 79-characters long", >> lineno) > > > I believe there are other examples that would hit this same problem, such > as *.am at least. flake8 is already checking the equivalent for Python > files. An alternative would be to just whitelist what we want to check. > (.c, .h, .md?) I think either a whitelist or blacklist work fine for this check. There are certain files which just don't make sense to check for line lengths. That said, the proposed patch looks good to me.
On 5 April 2016 at 11:37, Russell Bryant <russell@ovn.org> wrote: > > > On Tue, Apr 5, 2016 at 2:17 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> > > Some thoughts below ... > >> --- >> utilities/checkpatch.py | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py >> index d9011f370816..791c7d902fa5 100755 >> --- a/utilities/checkpatch.py >> +++ b/utilities/checkpatch.py >> @@ -154,7 +154,10 @@ 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: >> + >> + # Don't enforce character limit on test files, since they >> commonly >> + # include long pieces of text like flows or hex dumps of >> packets >> + if len(line[1:]) > 79 and '.at' not in current_file: >> print_line = True >> print_warning("Line is greater than 79-characters long", >> lineno) > > > I believe there are other examples that would hit this same problem, such as > *.am at least. flake8 is already checking the equivalent for Python files. > An alternative would be to just whitelist what we want to check. (.c, .h, > .md?) Hmm. A whitelist wouldn't cover things like NEWS, NOTICE, debian/changelog, rhel/README.RHEL, etc. I'm leaning towards blacklisting files with the following strings in their name at the moment: [ '.am', '.at', 'etc', '.in', '.mk', '.patch', '.py']
On 6 April 2016 at 07:33, Aaron Conole <aconole@redhat.com> wrote: > Russell Bryant <russell@ovn.org> writes: > >> On Tue, Apr 5, 2016 at 2:17 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> >> >> Some thoughts below ... >> >> --- >>> utilities/checkpatch.py | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py >>> index d9011f370816..791c7d902fa5 100755 >>> --- a/utilities/checkpatch.py >>> +++ b/utilities/checkpatch.py >>> @@ -154,7 +154,10 @@ 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: >>> + >>> + # Don't enforce character limit on test files, since they >>> commonly >>> + # include long pieces of text like flows or hex dumps of >>> packets >>> + if len(line[1:]) > 79 and '.at' not in current_file: >>> print_line = True >>> print_warning("Line is greater than 79-characters long", >>> lineno) >> >> >> I believe there are other examples that would hit this same problem, such >> as *.am at least. flake8 is already checking the equivalent for Python >> files. An alternative would be to just whitelist what we want to check. >> (.c, .h, .md?) > > I think either a whitelist or blacklist work fine for this check. There are > certain files which just don't make sense to check for line > lengths. > > That said, the proposed patch looks good to me. Thanks for the reviews, I'll send a follow up (in case my python style is a bit too C-like ;) )
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index d9011f370816..791c7d902fa5 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -154,7 +154,10 @@ 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: + + # Don't enforce character limit on test files, since they commonly + # include long pieces of text like flows or hex dumps of packets + if len(line[1:]) > 79 and '.at' not in current_file: print_line = True print_warning("Line is greater than 79-characters long", lineno)
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> --- utilities/checkpatch.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)