[libcamera-devel] [PATCH v2 2/6] checkstyle: Exit with 1 status if issues are found

[Laurent Pinchart]

Hi Nicolas,

On Fri, Jan 17, 2020 at 05:07:40PM -0500, Nicolas Dufresne wrote:
> Le vendredi 17 janvier 2020 à 23:58 +0200, Laurent Pinchart a écrit :
> > On Fri, Jan 17, 2020 at 02:17:29PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > 
> > > Makes the tool return 1 if there is any potential issues. This is
> > > needed when using this tool for pre-commit hook in order to abort
> > > the commit process.
> > > 
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > ---
> > >  utils/checkstyle.py | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >  mode change 100755 => 100644 utils/checkstyle.py
> > > 
> > > diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> > > old mode 100755
> > > new mode 100644
> > > index 7edea25..e7375b3
> > > --- a/utils/checkstyle.py
> > > +++ b/utils/checkstyle.py
> > > @@ -541,7 +541,7 @@ def check_style(top_level, commit):
> > >      files = [f for f in files if len([p for p in patterns if
> > > fnmatch.fnmatch(os.path.basename(f), p)])]
> > >      if len(files) == 0:
> > >          print("Commit doesn't touch source files, skipping")
> > > -        return
> > > +        return 0
> > >  
> > >      issues = 0
> > >      for f in files:
> > > @@ -554,6 +554,8 @@ def check_style(top_level, commit):
> > >          print("%u potential style %s detected, please review" % \
> > >                  (issues, 'issue' if issues == 1 else 'issues'))
> > >  
> > > +    return issues
> > > +
> > >  
> > >  def extract_revlist(revs):
> > >      """Extract a list of commits on which to operate from a revision or
> > > revision
> > > @@ -632,11 +634,12 @@ def main(argv):
> > >  
> > >      revlist = extract_revlist(args.revision_range)
> > >  
> > > +    issues = 0
> > >      for commit in revlist:
> > > -        check_style(top_level, commit)
> > > +        issues += check_style(top_level, commit)
> > >          print('')
> > >  
> > > -    return 0
> > > +    return min(issues, 1)
> > 
> > I find this a bit difficult to read, but the python alternative to the C
> > ternary operator would be
> > 
> > 	return issues and 1 or 0
> > 
> > which may not be more reable.
> > 
> > 	if issues:
> > 		return 1
> > 	else:
> > 		return 0
> > 
> > may be an option, but not very nice either. I'll leave it up to you,
> > maybe
> > 
> > 	# Return an error if any issues have been detected
> > 	return min(issues, 0)
> 
> One of the two last suggestion will do, I have no preference, but not the first
> one, that's just worst. Will you fix it why pushing ?

If you don't see a need to send a v3 due to other patches in this
series, I'll fix this (and possibly other small issues in other patches)
when pushing, yes.

> > 
> > In any case,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > >  
> > >  
> > >  if __name__ == '__main__':

-- 
Regards,

Laurent Pinchart